Bug 30033 – [chromium] DateExtension has reliability bot crashes

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[John Abd-El-Malek]

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.

[John Abd-El-Malek]

Created attachment 40557 [details]
Proposed patch

[Adam Barth]

Comment on attachment 40557 [details]
Proposed patch

Please use V8HiddenPropertyName to avoid hidden property name collisions.

[Adam Barth]

Also, we need a test.  :)

[John Abd-El-Malek]

Created attachment 40642 [details]
Updated per review comments

[John Abd-El-Malek]

This code is already tested by slow_unload_handler.html.  The crash happened on the reliability bot sporadically, and I couldn't repro it.

[Adam Barth]

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.

[Adam Barth]

(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?

[John Abd-El-Malek]

(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.

[Adam Barth]

(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?

[John Abd-El-Malek]

(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.

[Adam Barth]

> > > > 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.

[John Abd-El-Malek]

Created attachment 40684 [details]
Updated patch

[Adam Barth]

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.

[WebKit Commit Bot]

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.

[Adam Barth]

I'll land this by hand.

[Adam Barth]

Committed r49148: <http://trac.webkit.org/changeset/49148>

[John Abd-El-Malek]

(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.

[Adam Barth]

> 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.