Bug 78638 - Don't clear IntentRequest callback pointers on stop()
: Don't clear IntentRequest callback pointers on stop()
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 75123
  Show dependency treegraph
 
Reported: 2012-02-14 14:57 PST by
Modified: 2012-03-28 11:09 PST (History)


Attachments
Patch (2.81 KB, patch)
2012-02-14 14:58 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.42 KB, patch)
2012-02-15 11:21 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.38 KB, patch)
2012-02-17 17:39 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.28 KB, patch)
2012-02-21 10:48 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2012-02-21 11:41 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2012-02-23 11:44 PST, Greg Billock
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-02-14 14:57:05 PST
Don't clear IntentRequest callback pointers on stop()
------- Comment #1 From 2012-02-14 14:58:48 PST -------
Created an attachment (id=127053) [details]
Patch
------- Comment #2 From 2012-02-14 15:06:07 PST -------
(From update of attachment 127053 [details])
Please add a test that triggers this crash.
------- Comment #3 From 2012-02-15 11:21:29 PST -------
Created an attachment (id=127201) [details]
Patch
------- Comment #4 From 2012-02-15 11:24:57 PST -------
(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.
------- Comment #5 From 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 From 2012-02-15 11:32:02 PST -------
(In reply to comment #4)
> (From update of attachment 127201 [details] [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 From 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 From 2012-02-17 17:39:03 PST -------
Created an attachment (id=127680) [details]
Patch
------- Comment #9 From 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 From 2012-02-19 12:39:02 PST -------
(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.

> 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 From 2012-02-21 10:48:07 PST -------
Created an attachment (id=127997) [details]
Patch
------- Comment #12 From 2012-02-21 10:50:16 PST -------
(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?

>> 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 From 2012-02-21 10:59:29 PST -------
(In reply to comment #12)
> (From update of attachment 127680 [details] [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 From 2012-02-21 11:00:06 PST -------
(From update of attachment 127997 [details])
Looks great.  Thanks for working on the test.
------- Comment #15 From 2012-02-21 11:34:16 PST -------
(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.

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 From 2012-02-21 11:41:59 PST -------
Created an attachment (id=128012) [details]
Patch
------- Comment #17 From 2012-02-21 11:55:40 PST -------
ok, should be ready to go.
------- Comment #18 From 2012-02-23 11:39:20 PST -------
(From update of attachment 128012 [details])
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 From 2012-02-23 11:42:55 PST -------
(From update of attachment 128012 [details])
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 From 2012-02-23 11:44:01 PST -------
Created an attachment (id=128518) [details]
Patch
------- Comment #21 From 2012-02-23 20:58:15 PST -------
(From update of attachment 128518 [details])
Clearing flags on attachment: 128518

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