Bug 51357

Summary: REGRESSION (r66428/r71892): Crash after assertion failure (!m_reachedTerminalState) in ResourceLoader::didCancel()
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, beidson, commit-queue, hamaji, japhet, koivisto, mihaip, rtenneti, thestig
Priority: P1 Keywords: InRadar, Regression
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.6   
Attachments:
Description Flags
test case (will ASSERT)
none
Patch
none
Patch for landing none

Description Alexey Proskuryakov 2010-12-20 15:25:13 PST
Created attachment 77044 [details]
test case (will ASSERT)

Steps to reproduce:
1) Disable pop-up blocker.
2) Open attached test case.
3) When a print dialog appears, click Cancel.
Comment 1 Alexey Proskuryakov 2010-12-20 16:34:18 PST
The problem is that main resource can finish loading during execution of its own MainResourceLoader::didCancel():

    frameLoader()->receivedMainResourceError(error, true);
    ResourceLoader::didCancel(error);

Here, receivedMainResourceError() indirectly starts printing, and load finishes while a print sheet is displayed. Naturally, ResourceLoader::didCancel() isn't happy about the load being half-canceled and half-finished.

One may think (as I did) that adding PageGroupLoadDeferrer in Chrome::print() would solve this, but DocumentLoader doesn't have its MainResourceLoader pointer at this point any more, so deferring the page doesn't defer its main resource.
Comment 2 Alexey Proskuryakov 2011-01-22 01:54:13 PST
Actually, this crashes nightlies.
Comment 3 Alexey Proskuryakov 2011-01-22 01:54:40 PST
<rdar://problem/8903430>
Comment 4 raman tenneti 2011-01-22 10:50:22 PST
*** Bug 52945 has been marked as a duplicate of this bug. ***
Comment 5 raman tenneti 2011-01-22 10:59:14 PST
Opening the attached file crashes with Google chrome 8.0.552.237. thanks much.
Comment 6 Alexey Proskuryakov 2011-01-22 15:21:17 PST
This crash started with r71892. Before that, the print dialog didn't appear at all, which was a bug introduced in r66428.

Sounds like a proper fix would involve improving deferred window.print() behavior.
Comment 7 Adam Barth 2011-03-07 23:36:24 PST
I have this in the debugger.  Looking a potential solutions...


#15	0x100fc4d3a in CallDelegate at WebDelegateImplementationCaching.mm:103
#16	0x100fc4def in CallUIDelegate at WebDelegateImplementationCaching.mm:439
#17	0x100fb4b79 in WebChromeClient::print at WebChromeClient.mm:662
#18	0x10164ebbc in WebCore::Chrome::print at Chrome.cpp:422
#19	0x1018b4c0d in WebCore::DOMWindow::print at DOMWindow.cpp:943
#20	0x1018b4c3d in WebCore::DOMWindow::finishedLoading at DOMWindow.cpp:1623
#21	0x1017f6cf2 in WebCore::DocumentLoader::updateLoading at DocumentLoader.cpp:376
#22	0x1017f6ef0 in WebCore::DocumentLoader::setPrimaryLoadComplete at DocumentLoader.cpp:426
#23	0x101978d95 in WebCore::FrameLoader::mainReceivedCompleteError at FrameLoader.cpp:3302
#24	0x1017f73cb in WebCore::DocumentLoader::mainReceivedError at DocumentLoader.cpp:205
#25	0x101976626 in WebCore::FrameLoader::receivedMainResourceError at FrameLoader.cpp:2841
#26	0x101e9f513 in WebCore::MainResourceLoader::didCancel at MainResourceLoader.cpp:110
#27	0x1020f85c5 in WebCore::ResourceLoader::cancel at ResourceLoader.cpp:381
#28	0x1020f8220 in WebCore::ResourceLoader::cancel at ResourceLoader.cpp:371
#29	0x1017f8658 in WebCore::DocumentLoader::stopLoading at DocumentLoader.cpp:248
#30	0x1019734dc in WebCore::FrameLoader::stopAllLoaders at FrameLoader.cpp:1717
#31	0x101978bfa in WebCore::FrameLoader::stopForUserCancel at FrameLoader.cpp:1735
#32	0x100fd56c8 in -[WebFrame stopLoading] at WebFrame.mm:1573
#33	0x1010857c3 in -[WebView(WebIBActions) stopLoading:] at WebView.mm:4197
#34	0x10008522f in ??
#35	0x100fb3bc8 in WebChromeClient::closeWindowSoon at WebChromeClient.mm:479
#36	0x10164eb3c in WebCore::Chrome::closeWindowSoon at Chrome.cpp:280
#37	0x1018b4df5 in WebCore::DOMWindow::close at DOMWindow.cpp:926
#38	0x101c37e84 in WebCore::jsDOMWindowPrototypeFunctionClose at JSDOMWindow.cpp:9274
Comment 8 Adam Barth 2011-03-08 00:42:11 PST
Apparently this is a top crash:
http://code.google.com/p/chromium/issues/detail?id=63783
Comment 9 Adam Barth 2011-03-08 00:54:14 PST
Created attachment 85033 [details]
Patch
Comment 10 Adam Barth 2011-03-08 00:55:09 PST
I'm not entirely sure this is the right fix.  It's somewhat of a targeted fix.  Maybe something more general is better?
Comment 11 Mihai Parparita 2011-03-08 06:30:23 PST
Comment on attachment 85033 [details]
Patch

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

> LayoutTests/printing/print-close-crash.html:10
> +w.print();

Based on previous patches around window.print(), it's not supported by the DRT, so I'm not sure this layout test is actually testing anything:

http://trac.webkit.org/changeset/66428
http://trac.webkit.org/changeset/71892
Comment 12 Darin Adler 2011-03-08 08:37:32 PST
Comment on attachment 85033 [details]
Patch

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

> Source/WebCore/page/DOMWindow.cpp:1632
> +        m_printTimer.stop();
> +        m_printTimer.startOneShot(0);

There is no need to call stop before start on a Timer. Calling any version of start automatically cancels any previously-scheduler timer as stop does.
Comment 13 Alexey Proskuryakov 2011-03-08 09:15:54 PST
What will this do if window.close() is called right after window.print()? It's pretty common for Web pages to call window.open()/print()/close() in one script. I'm not sure if a timer will result in correct behavior.

Please also try less practical variations of this (e.g. setTimeout("window.close()", 0); calling window.close() before window.print(); opening a separate window or not; testing whether the whole page actually gets parsed before printing). I've been getting some crashes and assertion failures even without your patch - although my build is old, and some may have been fixed.
Comment 14 Alexey Proskuryakov 2011-03-08 09:29:52 PST
Also, interaction with any work performed in onload could cause incompatibilities, perhaps even practical ones.

It seems like we're adding layers of patches without fully understanding what other browsers do, and without support from HTML5. Unsurprisingly, this hasn't worked so well.
Comment 15 Adam Barth 2011-03-08 12:28:16 PST
> Based on previous patches around window.print(), it's not supported by the DRT, so I'm not sure this layout test is actually testing anything:

I'll try running it without the patch to check.

> There is no need to call stop before start on a Timer. Calling any version of start automatically cancels any previously-scheduler timer as stop does.

Ok.  I was modeling this off other code that does this, but that code is probably confused.

> What will this do if window.close() is called right after window.print()? It's pretty common for Web pages to call window.open()/print()/close() in one script. I'm not sure if a timer will result in correct behavior.

That's the test case for this bug.  It seems to fix the problem.

> Please also try less practical variations of this (e.g. setTimeout("window.close()", 0); calling window.close() before window.print(); opening a separate window or not; testing whether the whole page actually gets parsed before printing). I've been getting some crashes and assertion failures even without your patch - although my build is old, and some may have been fixed.

Hum...  I'm not entirely sure what you have in mind here.  The issue in this bug is that calling DOMWindow::print synchronously from inside a load cancel spins an event loop and leaves the loader in an inconsistent state.  This patch just makes the call async so we don't spin an event loop inside the cancel handler.

Do you mean there are other bugs we should try to address at the same time?

> Also, interaction with any work performed in onload could cause incompatibilities, perhaps even practical ones.

That's certainly possible.  Do you have a suggestion for another approach to fixing this bug?  We might want to try the fix and see if we have compat problems.

> It seems like we're adding layers of patches without fully understanding what other browsers do, and without support from HTML5. Unsurprisingly, this hasn't worked so well.

I definitely get that sense too.  Unfortunately, we don't even seem to have good test coverage of the changes we're making.  I certainly don't have global understanding of how this all should work...  How do you think we should proceed?
Comment 16 Alexey Proskuryakov 2011-03-08 12:58:25 PST
As mentioned in comment 1, my initial thought was to add PageGroupLoadDeferrer - maybe there is a way to make it work, after all? Right now, I do not remember why it seemed like that it could help, I could be just wrong.

> That's the test case for this bug.

I confused two cases, sorry. The test is for the common case where a separate window is opened, but I was thinking about an inline script in a document that's being loaded?

<body>
aaa
<script>
window.print();
window.close();
</script>
bbb
</body>

> Do you mean there are other bugs we should try to address at the same time?

I was more concerned about the possibility of further diverging from other browsers and introducing new bugs.

The patch has an r+, so you should feel free to land it at your discretion. It would be good to investigate this more deeply, and I planned to work on this bug fairly soon myself. Since this is largely about interaction between loader and parser, you could be an even better person to investigate, if you have the interest.
Comment 17 Mihai Parparita 2011-03-08 13:05:25 PST
(In reply to comment #16)
> <script>
> window.print();
> window.close();
> </script>

Since we were already deferring print during loading (with http://trac.webkit.org/changeset/66428), even before this patch the window.close() would run before window.print().
Comment 18 Alexey Proskuryakov 2011-03-08 14:26:38 PST
The tricky part is that window.close() indirectly calls finishedLoading(), so printing still happens before close() actually closes the window.
Comment 19 Adam Barth 2011-03-10 00:42:53 PST
Comment on attachment 85033 [details]
Patch

It turns out the test case doesn't work in DRT.  I've left it in the patch, but added manual test too.
Comment 20 Adam Barth 2011-03-10 00:57:06 PST
> I confused two cases, sorry. The test is for the common case where a separate window is opened, but I was thinking about an inline script in a document that's being loaded?
> 
> <body>
> aaa
> <script>
> window.print();
> window.close();
> </script>
> bbb
> </body>

That test case seems to work fine.  I haven't included it in the patch.
Comment 21 Adam Barth 2011-03-10 00:58:31 PST
As much as I feel bad about piling onto this code without fully understanding all the cases, I'm tempted by the siren song of fixing this particular crash.  I'm going to land this patch (with the added test).  Alexey, if you'd be willing to work on this system more, I'm sure everyone would be appreciative.
Comment 22 Adam Barth 2011-03-10 01:00:16 PST
Created attachment 85294 [details]
Patch for landing
Comment 23 Alexey Proskuryakov 2011-03-10 17:04:34 PST
I agree that this patch improves the situation by fixing a top crasher. I won't be able to justify it to myself investigating behavior differences any time soon - unless this patch causes some horrible regression :)
Comment 24 WebKit Commit Bot 2011-03-10 21:07:39 PST
Comment on attachment 85294 [details]
Patch for landing

Clearing flags on attachment: 85294

Committed r80812: <http://trac.webkit.org/changeset/80812>
Comment 25 WebKit Commit Bot 2011-03-10 21:07:44 PST
All reviewed patches have been landed.  Closing bug.
Comment 26 Alexey Proskuryakov 2011-04-22 15:52:16 PDT
This did cause a horrible regression, so I'm re-fixing this in a different way in bug 59241.