See http://code.google.com/p/chromium/issues/detail?id=23043 Mads helped me track this down, looks like the weak persistent handle approach is problematic since the JS function has no one else grabbing a reference to it, so it can be destroyed during GC when the other JS enableSleepDetection function pointers are being called. This leads to problem since during iteration the vector can change. A simpler solution is to add the function pointer as a hidden value on the Date constructor. This patch achieves that.
Created attachment 40557 [details] Proposed patch
Comment on attachment 40557 [details] Proposed patch Please use V8HiddenPropertyName to avoid hidden property name collisions.
Also, we need a test. :)
Created attachment 40642 [details] Updated per review comments
This code is already tested by slow_unload_handler.html. The crash happened on the reliability bot sporadically, and I couldn't repro it.
Comment on attachment 40642 [details] Updated per review comments We should add a test for the crash. + v8::Local<v8::Value> result = V8Proxy::retrieve()->evaluate(WebCore::ScriptSourceCode("Date"), 0); This seems overly complicated. Why not just get the "Date" property of the context's global object? Other than those two issue, this looks reasonable.
(In reply to comment #5) > This code is already tested by slow_unload_handler.html. The crash happened on > the reliability bot sporadically, and I couldn't repro it. Ah, didn't see this comment before. Have you tried forcing a GC while running the test?
(In reply to comment #6) > (From update of attachment 40642 [details]) > We should add a test for the crash. This wasn't a reproducible crash. I'm open to suggestions on how this crash can be reproduced. > > + v8::Local<v8::Value> result = > V8Proxy::retrieve()->evaluate(WebCore::ScriptSourceCode("Date"), 0); > > This seems overly complicated. Why not just get the "Date" property of the > context's global object? Mads had suggested this. It's useful in case the page replaced the Date object, in which case we don't want to override getTime since they could have changed its functionality (i.e. added extra parameters). > > Other than those two issue, this looks reasonable.
(In reply to comment #8) > This wasn't a reproducible crash. I'm open to suggestions on how this crash > can be reproduced. Did you try running GC explicitly at an opportune moment? I think there's a function to force GC around somewhere in LayoutTests. > > + v8::Local<v8::Value> result = > > V8Proxy::retrieve()->evaluate(WebCore::ScriptSourceCode("Date"), 0); > > > > This seems overly complicated. Why not just get the "Date" property of the > > context's global object? > > Mads had suggested this. It's useful in case the page replaced the Date > object, in which case we don't want to override getTime since they could have > changed its functionality (i.e. added extra parameters). How does this solve that problem? Won't evaling "Date" just give you the modified object?
(In reply to comment #9) > (In reply to comment #8) > > This wasn't a reproducible crash. I'm open to suggestions on how this crash > > can be reproduced. > > Did you try running GC explicitly at an opportune moment? I think there's a > function to force GC around somewhere in LayoutTests. Yeah I tried that, but the crash only happens if GC happens during the call to the enableSleepDetection function that's in the closure. Trying to simulate gc in the unload handlers themselves doesn't trigger this. > > > > + v8::Local<v8::Value> result = > > > V8Proxy::retrieve()->evaluate(WebCore::ScriptSourceCode("Date"), 0); > > > > > > This seems overly complicated. Why not just get the "Date" property of the > > > context's global object? > > > > Mads had suggested this. It's useful in case the page replaced the Date > > object, in which case we don't want to override getTime since they could have > > changed its functionality (i.e. added extra parameters). > > How does this solve that problem? Won't evaling "Date" just give you the > modified object? It prevents this problem because the modified "Date" object won't have the hidden property on it, so the code will early return.
> > > > This seems overly complicated. Why not just get the "Date" property of the > > > > context's global object? > > It prevents this problem because the modified "Date" object won't have the > hidden property on it, so the code will early return. Oh, I'm just saying that you replace the "eval the string Date" code with "get the Date property of the global object". I agree that you should continue to store the hidden property on the Date object itself.
Created attachment 40684 [details] Updated patch
Comment on attachment 40684 [details] Updated patch Thanks John. Normally we check the result of V8Proxy::retrieve()->context() to see if the context is empty (e.g., because JavaScript is disabled). I don't think we can get into that situation here, however.
Comment on attachment 40684 [details] Updated patch Rejecting patch 40684 from commit-queue. Patch https://bugs.webkit.org/attachment.cgi?id=40684 from bug 30033 failed to download and apply.
I'll land this by hand.
Committed r49148: <http://trac.webkit.org/changeset/49148>
(In reply to comment #16) > Committed r49148: <http://trac.webkit.org/changeset/49148> Thanks, btw for future reference I'm a committer so I don't need to burden anyone anymore.
> Thanks, btw for future reference I'm a committer so I don't need to burden > anyone anymore. Ah, I knew that, but forgot. You might want to add yourself to this page: http://trac.webkit.org/wiki/WebKit%20Team In any case, I knew what the merge conflict was because I r+ed the conflicting change, so it was easy for me to fix.