Bug 78638 – Don't clear IntentRequest callback pointers on stop()

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Greg Billock]

Don't clear IntentRequest callback pointers on stop()

[Greg Billock]

Created attachment 127053 [details]
Patch

[Adam Barth]

Comment on attachment 127053 [details]
Patch

Please add a test that triggers this crash.

[Greg Billock]

Created attachment 127201 [details]
Patch

[Adam Barth]

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.

[Greg Billock]

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.

[Greg Billock]

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

[Adam Barth]

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

[Greg Billock]

Created attachment 127680 [details]
Patch

[Greg Billock]

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.

[Adam Barth]

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.

[Greg Billock]

Created attachment 127997 [details]
Patch

[Greg Billock]

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.

[Adam Barth]

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

[Adam Barth]

Comment on attachment 127997 [details]
Patch

Looks great.  Thanks for working on the test.

[Greg Billock]

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.

[Greg Billock]

Created attachment 128012 [details]
Patch

[Greg Billock]

ok, should be ready to go.

[Adam Barth]

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.

[Greg Billock]

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.

[Greg Billock]

Created attachment 128518 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128518 [details]
Patch

Clearing flags on attachment: 128518

Committed r108724: <http://trac.webkit.org/changeset/108724>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.