Added comment explaining the problem with GetScriptWrapper calling GC. On Mon, Oct 6, 2008 at 3:02 PM, Kasper Lund <[EMAIL PROTECTED]> wrote:
> ... 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 -~----------~----~----~----~------~----~------~--~---
