Bug 78638

Summary: Don't clear IntentRequest callback pointers on stop()
Product: WebKit Reporter: Greg Billock <gbillock>
Component: New BugsAssignee: Greg Billock <gbillock>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 75123    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Greg Billock 2012-02-14 14:57:05 PST
Don't clear IntentRequest callback pointers on stop()
Comment 1 Greg Billock 2012-02-14 14:58:48 PST
Created attachment 127053 [details]
Patch
Comment 2 Adam Barth 2012-02-14 15:06:07 PST
Comment on attachment 127053 [details]
Patch

Please add a test that triggers this crash.
Comment 3 Greg Billock 2012-02-15 11:21:29 PST
Created attachment 127201 [details]
Patch
Comment 4 Adam Barth 2012-02-15 11:24:57 PST
Comment on attachment 127201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127201&action=review

> LayoutTests/webintents/resources/web-intents-reload-new.html:6
> +      window.setTimeout(1000, function() {
> +        });

This is probably not the right way to test this.

> LayoutTests/webintents/web-intents-reload.html:20
> +        window.setTimeout(1000, function() {
> +          debug("* survived");
> +        });

You shouldn't need any timeouts.  Maybe try using an iframe and triggering the crash in side the iframe.  You can have the parent frame grab a pointer to the iframe's document to prevent the document from getting deleted while the test is running.
Comment 5 Greg Billock 2012-02-15 11:29:07 PST
Uploaded a new patch with a test.

Here's the situation now: a couple of days ago, the bug reliably happened on any page reloaded after an intent had been triggered. I also got chromium to reproduce the crash on the test pretty much as submitted here a couple of times.

However DumpRenderTree never triggered the crash, and now ToT chromium doesn't reproduce the crash at all either, on the test or on the original bug report reproduction case.

I still think the fix is an appropriate one, but either the stopActiveDOMObjects call context has changed a bit, or it was always racy with destruction of these objects, and something in my environment makes it now very unlikely to trigger.

I think the fix in IntentRequest is the right one, but given the flakiness, I'm not sure the test actually prevents regression. Is there a way a test can trigger ScriptExecutionController::stopActiveDOMObjects directly? If so, that should prove it explicitly. I'm currently doing that with a page load (which is what happened in the original bug report).

Background: the diagnosis of the problem is that the crash happened with IntentRequest::stop() was called, which led to derefs of the IntentResultCallback objects, which then wanted to delete themselves. But the V8 implementation uses ActiveDOMObject internally for these callbacks, which on delete call ScriptExecutionContext::willDestroyActiveDOMObject and crash there, since the iteration was in process.
Comment 6 Greg Billock 2012-02-15 11:32:02 PST
(In reply to comment #4)
> (From update of attachment 127201 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127201&action=review
> 
> > LayoutTests/webintents/resources/web-intents-reload-new.html:6
> > +      window.setTimeout(1000, function() {
> > +        });
> 
> This is probably not the right way to test this.
> 
> > LayoutTests/webintents/web-intents-reload.html:20
> > +        window.setTimeout(1000, function() {
> > +          debug("* survived");
> > +        });
> 
> You shouldn't need any timeouts.  Maybe try using an iframe and triggering the crash in side the iframe.  You can have the parent frame grab a pointer to the iframe's document to prevent the document from getting deleted while the test is running.

Yeah, the timeouts were an attempt to keep the SEC alive to defeat whatever race might be going on. So the theory is that if I keep a document pointer to an iframe, then setting the location of the iframe should cause the stopActiveDOMObjects before any deletions? If that'll be different than the sequencing when using the main frame, it might work.
Comment 7 Adam Barth 2012-02-15 13:15:08 PST
> Yeah, the timeouts were an attempt to keep the SEC alive to defeat whatever race might be going on. So the theory is that if I keep a document pointer to an iframe, then setting the location of the iframe should cause the stopActiveDOMObjects before any deletions?

Yes.  More specifically, you should keep a pointer to the frame's document (e.g., frames[0].document).

> If that'll be different than the sequencing when using the main frame, it might work.

If you need to use a main frame, you can use window.open to create one.  However, using an iframe is better for testing.
Comment 8 Greg Billock 2012-02-17 17:39:03 PST
Created attachment 127680 [details]
Patch
Comment 9 Greg Billock 2012-02-17 17:40:11 PST
OK, This needed some test infrastructure to pass, but that will be useful in other tests of the feature. Test now crashes repeatably without fix and passes with the fix.
Comment 10 Adam Barth 2012-02-19 12:39:02 PST
Comment on attachment 127680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127680&action=review

> Source/WebCore/ChangeLog:-3004
> ->>>>>>> .r108008

This change looks spurious.

> LayoutTests/webintents/resources/web-intents-reload-new.html:7
> +<html>
> +  <head>
> +  </head>
> +  <body>
> +    <p>No crash is pass.</p>
> +  </body>
> +</html>

You can also just call this "pass.html" and just have the text PASS (so it can be reused by other tests).

> LayoutTests/webintents/resources/web-intents-reload-orig.html:23
> +        // This should not crash.
> +        window.location = "resources/web-intents-reload-new.html";

Are you use this shouldn't be "web-intents-reload-new.html" ?  The URL should be relative to this document.
Comment 11 Greg Billock 2012-02-21 10:48:07 PST
Created attachment 127997 [details]
Patch
Comment 12 Greg Billock 2012-02-21 10:50:16 PST
Comment on attachment 127680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127680&action=review

>> Source/WebCore/ChangeLog:-3004
>> ->>>>>>> .r108008
> 
> This change looks spurious.

I figured this was accidental and part of a merge. Shall I put it back?

>> LayoutTests/webintents/resources/web-intents-reload-new.html:7
>> +</html>
> 
> You can also just call this "pass.html" and just have the text PASS (so it can be reused by other tests).

Done.

>> LayoutTests/webintents/resources/web-intents-reload-orig.html:23
>> +        window.location = "resources/web-intents-reload-new.html";
> 
> Are you use this shouldn't be "web-intents-reload-new.html" ?  The URL should be relative to this document.

I tried it that way when I first wrote the test, but it didn't work. I think there's something about the DumpRenderTree test environment I don't grok.
Comment 13 Adam Barth 2012-02-21 10:59:29 PST
(In reply to comment #12)
> (From update of attachment 127680 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127680&action=review
> 
> >> Source/WebCore/ChangeLog:-3004
> >> ->>>>>>> .r108008
> > 
> > This change looks spurious.
> 
> I figured this was accidental and part of a merge. Shall I put it back?

We can clean up the ChangeLog file, but we should do that in a separate patch.

> >> LayoutTests/webintents/resources/web-intents-reload-orig.html:23
> >> +        window.location = "resources/web-intents-reload-new.html";
> > 
> > Are you use this shouldn't be "web-intents-reload-new.html" ?  The URL should be relative to this document.
> 
> I tried it that way when I first wrote the test, but it didn't work. I think there's something about the DumpRenderTree test environment I don't grok.

Ah, probably because you're calling between frames and window.location (insanely) uses the first script to resolve relative URLs.
Comment 14 Adam Barth 2012-02-21 11:00:06 PST
Comment on attachment 127997 [details]
Patch

Looks great.  Thanks for working on the test.
Comment 15 Greg Billock 2012-02-21 11:34:16 PST
Comment on attachment 127680 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127680&action=review

>>>> Source/WebCore/ChangeLog:-3004
>>>> ->>>>>>> .r108008
>>> 
>>> This change looks spurious.
>> 
>> I figured this was accidental and part of a merge. Shall I put it back?
> 
> We can clean up the ChangeLog file, but we should do that in a separate patch.

ok

>>>> LayoutTests/webintents/resources/web-intents-reload-orig.html:23
>>>> +        window.location = "resources/web-intents-reload-new.html";
>>> 
>>> Are you use this shouldn't be "web-intents-reload-new.html" ?  The URL should be relative to this document.
>> 
>> I tried it that way when I first wrote the test, but it didn't work. I think there's something about the DumpRenderTree test environment I don't grok.
> 
> Ah, probably because you're calling between frames and window.location (insanely) uses the first script to resolve relative URLs.

That explains it.
Comment 16 Greg Billock 2012-02-21 11:41:59 PST
Created attachment 128012 [details]
Patch
Comment 17 Greg Billock 2012-02-21 11:55:40 PST
ok, should be ready to go.
Comment 18 Adam Barth 2012-02-23 11:39:20 PST
Comment on attachment 128012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128012&action=review

> Tools/DumpRenderTree/chromium/WebViewHost.h:239
> +    virtual void dispatchIntent(WebKit::WebFrame* source, const WebKit::WebIntentRequest&) OVERRIDE;

We actually don't use OVERRIDE in for the WebKit API to make it easier to change.  That's mostly a concern for cross-repository OVERRIDE declarations though.
Comment 19 Greg Billock 2012-02-23 11:42:55 PST
Comment on attachment 128012 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128012&action=review

>> Tools/DumpRenderTree/chromium/WebViewHost.h:239
>> +    virtual void dispatchIntent(WebKit::WebFrame* source, const WebKit::WebIntentRequest&) OVERRIDE;
> 
> We actually don't use OVERRIDE in for the WebKit API to make it easier to change.  That's mostly a concern for cross-repository OVERRIDE declarations though.

Removed.
Comment 20 Greg Billock 2012-02-23 11:44:01 PST
Created attachment 128518 [details]
Patch
Comment 21 WebKit Review Bot 2012-02-23 20:58:15 PST
Comment on attachment 128518 [details]
Patch

Clearing flags on attachment: 128518

Committed r108724: <http://trac.webkit.org/changeset/108724>
Comment 22 WebKit Review Bot 2012-02-23 20:58:21 PST
All reviewed patches have been landed.  Closing bug.