Bug 146705 - prevent load errors with invalid requests
Summary: prevent load errors with invalid requests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-07-07 17:40 PDT by Alex Christensen
Modified: 2015-07-09 11:37 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.57 KB, patch)
2015-07-07 17:41 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (531.63 KB, application/zip)
2015-07-07 18:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (575.99 KB, application/zip)
2015-07-07 18:20 PDT, Build Bot
no flags Details
Patch (3.65 KB, patch)
2015-07-08 11:55 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (7.52 KB, patch)
2015-07-08 18:21 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (5.13 KB, patch)
2015-07-08 18:32 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.98 KB, patch)
2015-07-09 10:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (8.97 KB, patch)
2015-07-09 10:40 PDT, Alex Christensen
beidson: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2015-07-07 17:40:08 PDT
ResourceLoader::cannotShowURLError() uses m_request.  In ResourceLoader::willSendRequest it is not set before cannotShowURLError is called.  This is causing crashes, especially with content blocking.
Comment 1 Alex Christensen 2015-07-07 17:41:44 PDT
Created attachment 256338 [details]
Patch
Comment 2 Oliver Hunt 2015-07-07 17:47:01 PDT
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.
Comment 3 Oliver Hunt 2015-07-07 17:47:59 PDT
CC'ing brady as he knows about these things
Comment 4 Build Bot 2015-07-07 18:14:46 PDT
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
Comment 5 Build Bot 2015-07-07 18:14:49 PDT
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
Comment 6 Build Bot 2015-07-07 18:20:06 PDT
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
Comment 7 Build Bot 2015-07-07 18:20:09 PDT
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
Comment 8 Brady Eidson 2015-07-07 20:42:59 PDT
Out of curiosity, is there a rdar: for this one?
Comment 9 Oliver Hunt 2015-07-08 08:34:18 PDT
<rdar://problem/20855444> Vector overflow in loader code seems to occur when declarative blocks resource
Comment 10 Oliver Hunt 2015-07-08 08:36:10 PDT
Derp.

I'm not actually convinced that this null deref is actually the root of the problem, but maybe it is?
Comment 11 Oliver Hunt 2015-07-08 08:46:09 PDT
(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.
Comment 12 Alex Christensen 2015-07-08 11:55:07 PDT
Created attachment 256391 [details]
Patch
Comment 13 Alex Christensen 2015-07-08 18:21:48 PDT
Created attachment 256443 [details]
Patch
Comment 14 Alex Christensen 2015-07-08 18:32:46 PDT
Created attachment 256444 [details]
Patch
Comment 15 Brady Eidson 2015-07-08 20:31:52 PDT
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.
Comment 16 Alex Christensen 2015-07-09 10:31:15 PDT
Created attachment 256485 [details]
Patch
Comment 17 Chris Dumez 2015-07-09 10:38:38 PDT
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.
Comment 18 Alex Christensen 2015-07-09 10:40:03 PDT
Created attachment 256487 [details]
Patch
Comment 19 WebKit Commit Bot 2015-07-09 11:25:14 PDT
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
Comment 20 Alex Christensen 2015-07-09 11:37:55 PDT
http://trac.webkit.org/changeset/186597