... and I would add a comment on why you can't just fold the code together like it was originally...
On Mon, Oct 6, 2008 at 3:01 PM, Kasper Lund <[EMAIL PROTECTED]> wrote: > LGTM. I would use assignment for the initialization of 'wrapper', but > that's probably just me. > > On Mon, Oct 6, 2008 at 2:58 PM, <[EMAIL PROTECTED]> wrote: >> Reviewers: Kasper Lund, >> >> Description: >> Fixed unsafe code where a GC could occour after a Handle had been >> deferenced. >> >> instances->set(i, *GetScriptWrapper(script)); >> >> GetScriptWrapper can call GC. The failure have only been seen on ARM, >> where >> the g++ compiler pulls out the object from the instances handle to a >> register >> before calling GetScriptWrapper causing set to be called on an object >> which >> may have moved. >> >> Marked a test on ARM as no longer flaky, whereas two other fails >> consistently >> but that is no longer related to the problem fixed above. >> >> BUG=1308895 >> >> Please review this at http://codereview.chromium.org/6271 >> >> Affected files: >> M src/runtime.cc >> M test/mjsunit/mjsunit.status >> >> >> Index: test/mjsunit/mjsunit.status >> =================================================================== >> --- test/mjsunit/mjsunit.status (revision 443) >> +++ test/mjsunit/mjsunit.status (working copy) >> @@ -57,11 +57,7 @@ >> debug-step-stub-callfunction: FAIL >> debug-stepin-constructor: FAIL >> debug-step: FAIL >> -regress/regress-998565: FAIL >> - >> -# Bug number 1308895: These tests pass on the ARM simulator, but >> -# fail on the ARM Linux machine. >> -debug-script-breakpoints: PASS || FAIL >> -debug-scripts-request: PASS || FAIL >> +debug-script-breakpoints: FAIL >> debug-breakpoints: PASS || FAIL >> >> +regress/regress-998565: FAIL >> Index: src/runtime.cc >> =================================================================== >> --- src/runtime.cc (revision 443) >> +++ src/runtime.cc (working copy) >> @@ -4698,7 +4698,8 @@ >> // Convert the script objects to proper JS objects. >> for (int i = 0; i < count; i++) { >> Handle<Script> script(Script::cast(instances->get(i))); >> - instances->set(i, *GetScriptWrapper(script)); >> + Handle<JSValue> wrapper(GetScriptWrapper(script)); >> + instances->set(i, *wrapper); >> } >> >> // Return result as a JS array. >> >> >> > --~--~---------~--~----~------------~-------~--~----~ v8-dev mailing list [email protected] http://groups.google.com/group/v8-dev -~----------~----~----~----~------~----~------~--~---
