Bug 78638

Created attachment 127053 [details] Patch

Comment on attachment 127053 [details] Patch Please add a test that triggers this crash.

Created attachment 127201 [details] Patch

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.

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.

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

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

Created attachment 127680 [details] Patch

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

Created attachment 127997 [details] Patch

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.

(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 on attachment 127997 [details] Patch Looks great. Thanks for working on the test.

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.

Created attachment 128012 [details] Patch

ok, should be ready to go.

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

Created attachment 128518 [details] Patch

Comment on attachment 128518 [details] Patch Clearing flags on attachment: 128518 Committed r108724: <http://trac.webkit.org/changeset/108724>

All reviewed patches have been landed. Closing bug.