Bug 60796 – Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoading:]

• Component: Page Loading
• Status: RESOLVED FIXED

[Brady Eidson]

Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoading:]

An internal Apple app was calling [WebView stopLoading:] from within the didFailLoading: delegate for the mainResource, which was re-entering ResourceLoader::didCancel() causing a crash.

A reasonable refactor of ResourceLoader and its subclasses makes this go away and is a positive addition to the loader code, for once!

In radar as <rdar://problem/9366728>

[Brady Eidson]

Created attachment 93500 [details]
Patch v1

[WebKit Review Bot]

Attachment 93500 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 93500 [details]
Patch v1

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

> Source/WebCore/loader/ResourceLoader.cpp:359
> +    // off without re-running willCancel()

Missing period.

> Source/WebCore/loader/ResourceLoader.cpp:369
> +        m_cancelled = true;

For callers of the cancelled() function, it might be better m_cancelled was set to true even before calling willCancel. However, this is consistent with the behavior we had before. I suggest we follow up by moving the m_cancelled setting to before willCancel, which will require changing m_calledWillCancel around a bit too. Lets do that in a followup, though.

[Brady Eidson]

(In reply to comment #2)
> Attachment 93500 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
> 
> Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
> Total errors found: 1 in 9 files

Wrong!

>  If any of these errors are false positives, please file a bug against check-webkit-style.

Sure thing:
https://bugs.webkit.org/show_bug.cgi?id=60801

[WebKit Commit Bot]

Comment on attachment 93500 [details]
Patch v1

Rejecting attachment 93500 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'build-..." exit_code: 2

Last 500 characters of output:
.............................................................................................
http/tests/xmlhttprequest/xmlhttprequest-test-send-flag.html -> crashed
...
http/tests/xmlhttprequest/web-apps ...............
http/tests/xmlhttprequest/workers ...........
http/tests/xmlviewer .
http/tests/xmlviewer/dumpAsText ...........
865.93s total testing time

23510 test cases (99%) succeeded
7 test cases (<1%) had incorrect layout
2 test cases (<1%) crashed
15 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/8701014

[WebKit Commit Bot]

Created attachment 93512 [details]
Archive of layout-test-results from cr-jail-4

The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: cr-jail-4  Port: Mac  Platform: Mac OS X 10.6.7

[Brady Eidson]

Crashes and failures in http-land tests.  Running on SnowLeopard manually instead of relying on the commit queue.

[Brady Eidson]

Created attachment 93539 [details]
Patch v2

Patch v2, not for review.  We were sending the wrong error along, and had a misreplaced if (m_reachedTerminalState){} check.  That fixed all of the layout test *failures*....

But the (about 50/50 to reproduce) crasher in eventsource still occurs, as it involves calling in to javascript directly from the loader which is (unreliably) changing something and crashing stuff.

[Darin Adler]

(In reply to comment #8)
> But the (about 50/50 to reproduce) crasher in eventsource still occurs, as it involves calling in to javascript directly from the loader which is (unreliably) changing something and crashing stuff.

This EventSource crash is not happening for me on my Snow Leopard machine. I see no failures at all after multiple tests.

[WebKit Review Bot]

Attachment 93539 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 93539 [details]
Patch v2

Further testing indicates this works fine. We want to land it now.

[WebKit Commit Bot]

Comment on attachment 93539 [details]
Patch v2

Clearing flags on attachment: 93539

Committed r86720: <http://trac.webkit.org/changeset/86720>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.