Bug 146705 – prevent load errors with invalid requests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Alex Christensen]

ResourceLoader::cannotShowURLError() uses m_request.  In ResourceLoader::willSendRequest it is not set before cannotShowURLError is called.  This is causing crashes, especially with content blocking.

[Alex Christensen]

Created attachment 256338 [details]
Patch

[Oliver Hunt]

Comment on attachment 256338 [details]
Patch

What's the effect on this if something subsequently looks at the request? Previously m_request would remain null, but now there will be a request present.

[Oliver Hunt]

CC'ing brady as he knows about these things

[Build Bot]

Comment on attachment 256338 [details]
Patch

Attachment 256338 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5846646105571328

New failing tests:
http/tests/misc/will-send-request-returns-null-on-redirect.html

[Build Bot]

Created attachment 256341 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 256338 [details]
Patch

Attachment 256338 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5544045258473472

New failing tests:
http/tests/misc/will-send-request-returns-null-on-redirect.html

[Build Bot]

Created attachment 256342 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brady Eidson]

Out of curiosity, is there a rdar: for this one?

[Oliver Hunt]

<rdar://problem/20855444> Vector overflow in loader code seems to occur when declarative blocks resource

[Oliver Hunt]

Derp.

I'm not actually convinced that this null deref is actually the root of the problem, but maybe it is?

[Oliver Hunt]

(In reply to comment #10)
> Derp.
> 
> ... but maybe it is?

This is because we now auto clear RefPtrs so it's possible we were previously following a dead (unclear) RefPtr into the weeds in some way.

[Alex Christensen]

Created attachment 256391 [details]
Patch

[Alex Christensen]

Created attachment 256443 [details]
Patch

[Alex Christensen]

Created attachment 256444 [details]
Patch

[Brady Eidson]

Comment on attachment 256444 [details]
Patch

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

I think this change is great!

Layouttest?

> Source/WebCore/loader/ResourceLoader.cpp:325
> -
> +    

You should really configure Xcode to trim whitespace on empty lines.

[Alex Christensen]

Created attachment 256485 [details]
Patch

[Chris Dumez]

Comment on attachment 256485 [details]
Patch

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

> Source/WebCore/loader/NetscapePlugInStreamLoader.h:66
> +    virtual bool isPlugInStreamLoader() override final { return true; }

The class is already marked as final so having final here is redundant.

[Alex Christensen]

Created attachment 256487 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 256487 [details]
Patch

Rejecting attachment 256487 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 256487, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

/Volumes/Data/EWS/WebKit/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://webkit-queues.appspot.com/results/5704332095782912

[Alex Christensen]

http://trac.webkit.org/changeset/186597