Bug 195238 - Fix crashes when NetworkResourceLoader/NetworkLoadChecker are destroyed during HTTPS upgrade check
Summary: Fix crashes when NetworkResourceLoader/NetworkLoadChecker are destroyed durin...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-01 19:55 PST by Alex Christensen
Modified: 2019-03-04 12:55 PST (History)
3 users (show)

See Also:

Attachments
Patch (1.64 KB, patch)
2019-03-01 19:57 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2019-03-04 11:53 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Patch (2.72 KB, patch)
2019-03-04 12:19 PST, Alex Christensen 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-03-01 19:55:49 PST 
Fix crashes when NetworkResourceLoader/NetworkLoadChecker are destroyed during HTTPS upgrade check
Comment 1 Alex Christensen 2019-03-01 19:57:50 PST 
Created attachment 363402 [details]
Patch
Comment 2 Chris Dumez 2019-03-01 20:49:40 PST 
Comment on attachment 363402 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:227
> +    applyHTTPSUpgradeIfNeeded(WTFMove(request), [this, weakThis = makeWeakPtr(*this), client, handler = WTFMove(handler), originalRequest = WTFMove(originalRequest)](auto request) mutable {

Seems suspicious that you'd fix this here but not in the processContentExtensionRulesForLoad() lambda below...
Comment 3 Alex Christensen 2019-03-04 11:53:13 PST 
processContentExtensionRulesForLoad's implementation currently already has this protection, but there's no reason not to make this more robust.
Comment 4 Alex Christensen 2019-03-04 11:53:26 PST 
Created attachment 363534 [details]
Patch
Comment 5 Chris Dumez 2019-03-04 11:54:35 PST 
Comment on attachment 363534 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkLoadChecker.cpp:244
> +            if (!weakThis)

Personally, I would have put this below the if (!result.has_value()) check so that we may return a more accurate exception when we already have one.
Comment 6 Alex Christensen 2019-03-04 12:19:56 PST 
Created attachment 363539 [details]
Patch
Comment 7 WebKit Commit Bot 2019-03-04 12:54:50 PST 
Comment on attachment 363539 [details]
Patch

Clearing flags on attachment: 363539

Committed r242377: <https://trac.webkit.org/changeset/242377>
Comment 8 WebKit Commit Bot 2019-03-04 12:54:52 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 Radar WebKit Bug Importer 2019-03-04 12:55:38 PST 
<rdar://problem/48573506>