Bug 51357 – REGRESSION (r66428/r71892): Crash after assertion failure (!m_reachedTerminalState) in ResourceLoader::didCancel()

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

Actually, this crashes nightlies.

[Alexey Proskuryakov]

<rdar://problem/8903430>

[raman tenneti]

*** Bug 52945 has been marked as a duplicate of this bug. ***

[raman tenneti]

Opening the attached file crashes with Google chrome 8.0.552.237. thanks much.

[Alexey Proskuryakov]

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.

[Adam Barth]

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

[Adam Barth]

Apparently this is a top crash:
http://code.google.com/p/chromium/issues/detail?id=63783

[Adam Barth]

Created attachment 85033 [details]
Patch

[Adam Barth]

I'm not entirely sure this is the right fix.  It's somewhat of a targeted fix.  Maybe something more general is better?

[Mihai Parparita]

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

[Darin Adler]

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.

[Alexey Proskuryakov]

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.

[Alexey Proskuryakov]

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.

[Adam Barth]

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

[Alexey Proskuryakov]

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.

[Mihai Parparita]

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

[Alexey Proskuryakov]

The tricky part is that window.close() indirectly calls finishedLoading(), so printing still happens before close() actually closes the window.

[Adam Barth]

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.

[Adam Barth]

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

[Adam Barth]

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.

[Adam Barth]

Created attachment 85294 [details]
Patch for landing

[Alexey Proskuryakov]

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 :)

[WebKit Commit Bot]

Comment on attachment 85294 [details]
Patch for landing

Clearing flags on attachment: 85294

Committed r80812: <http://trac.webkit.org/changeset/80812>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This did cause a horrible regression, so I'm re-fixing this in a different way in bug 59241.