Bug 60796 - Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoading:]
Summary: Crash when code inside a ResourceLoadDelegate method calls [WebView stopLoadi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-13 13:29 PDT by Brady Eidson
Modified: 2011-05-17 16:42 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (13.45 KB, patch)
2011-05-13 13:31 PDT, Brady Eidson
darin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from cr-jail-4 (254.87 KB, application/zip)
2011-05-13 14:35 PDT, WebKit Commit Bot
no flags Details
Patch v2 (13.42 KB, patch)
2011-05-13 17:33 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2011-05-13 13:29:13 PDT
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>
Comment 1 Brady Eidson 2011-05-13 13:31:54 PDT
Created attachment 93500 [details]
Patch v1
Comment 2 WebKit Review Bot 2011-05-13 13:33:04 PDT
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.
Comment 3 Darin Adler 2011-05-13 13:35:04 PDT
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.
Comment 4 Brady Eidson 2011-05-13 13:55:29 PDT
(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
Comment 5 WebKit Commit Bot 2011-05-13 14:35:43 PDT
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
Comment 6 WebKit Commit Bot 2011-05-13 14:35:46 PDT
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
Comment 7 Brady Eidson 2011-05-13 14:46:19 PDT
Crashes and failures in http-land tests.  Running on SnowLeopard manually instead of relying on the commit queue.
Comment 8 Brady Eidson 2011-05-13 17:33:20 PDT
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.
Comment 9 Darin Adler 2011-05-16 09:49:09 PDT
(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.
Comment 10 WebKit Review Bot 2011-05-16 13:44:07 PDT
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.
Comment 11 Darin Adler 2011-05-17 14:41:36 PDT
Comment on attachment 93539 [details]
Patch v2

Further testing indicates this works fine. We want to land it now.
Comment 12 WebKit Commit Bot 2011-05-17 16:42:47 PDT
Comment on attachment 93539 [details]
Patch v2

Clearing flags on attachment: 93539

Committed r86720: <http://trac.webkit.org/changeset/86720>
Comment 13 WebKit Commit Bot 2011-05-17 16:42:53 PDT
All reviewed patches have been landed.  Closing bug.