Bug 144975 - REGRESSION (r179958): Crash in WebCore::DocumentLoader::detachFromFrame when -[id<WebPolicyDelegate> decidePolicyForMIMEType:request:frame:decisionListener:] fails to call -[id<WebPolicyDecisionListener> download|ignore|use]
Summary: REGRESSION (r179958): Crash in WebCore::DocumentLoader::detachFromFrame when ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-05-13 17:26 PDT by David Kilzer (:ddkilzer)
Modified: 2015-05-13 22:12 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (2.79 KB, patch)
2015-05-13 17:32 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2015-05-13 17:26:06 PDT
If an app overrides -decidePolicyForMIMEType:request:frame:decisionListener: in their id<WebPolicyDelegate> object and doesn't call one of three methods on id<WebPolicyDecisionListener> (-download, -ignore or -use), then the app will crash due to a RELEASE_ASSERT() added in r179958.

<http://trac.webkit.org/r179958>

This was not the intention of these RELEASE_ASSERT() changes (they were to catch the cause of a different crash which we think we found the cause of), so we can change these back to Debug-only ASSERT() statements.

Note that the app SHOULD be calling -download, -ignore or -use inside their -[id<WebPolicyDelegate> -decidePolicyForMIMEType:request:frame:decisionListener:] method, but it shouldn't cause a crash, and there's no easy way to fix this without further risk of compatibility issues (due to additional callbacks being made).
Comment 1 David Kilzer (:ddkilzer) 2015-05-13 17:26:21 PDT
<rdar://problem/20905401>
Comment 2 David Kilzer (:ddkilzer) 2015-05-13 17:32:57 PDT
Created attachment 253076 [details]
Patch v1
Comment 3 WebKit Commit Bot 2015-05-13 18:22:32 PDT
Comment on attachment 253076 [details]
Patch v1

Clearing flags on attachment: 253076

Committed r184323: <http://trac.webkit.org/changeset/184323>
Comment 4 WebKit Commit Bot 2015-05-13 18:22:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 5 Chris Dumez 2015-05-13 18:28:44 PDT
Thanks for taking care of this!
Comment 6 Brady Eidson 2015-05-13 22:08:28 PDT
(In reply to comment #0)
> Note that the app SHOULD be calling -download, -ignore or -use inside their
> -[id<WebPolicyDelegate>
> -decidePolicyForMIMEType:request:frame:decisionListener:] method, but it
> shouldn't cause a crash, and there's no easy way to fix this without further
> risk of compatibility issues (due to additional callbacks being made).

This is not actually true - It's perfectly legal for an app to hang on to the decision listener and call one of the three methods later, asynchronously.

That's why it's set up as a listener object instead of requiring a synchronous return value.
Comment 7 Brady Eidson 2015-05-13 22:12:13 PDT
Yah, these asserts are simply bogus in general.

If the client hangs on to the policy listener object for "awhile", say maybe to ask the user what they want to do, and the document is torn down in the meantime... DocumentLoader should handle that gracefully, release build or not.