Bug 185066 - PSON: Triggering a navigation to an invalid URL creates a new WebContent process
Summary: PSON: Triggering a navigation to an invalid URL creates a new WebContent process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-04-26 21:21 PDT by Ryosuke Niwa
Modified: 2018-04-27 15:30 PDT (History)
6 users (show)

See Also:


Attachments
Fixes the bug (5.91 KB, patch)
2018-04-26 21:43 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (13.07 MB, application/zip)
2018-04-26 23:29 PDT, EWS Watchlist
no flags Details
Patch for landing (5.84 KB, patch)
2018-04-27 14:31 PDT, Ryosuke Niwa
rniwa: commit-queue+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-04-26 21:21:53 PDT
When navigating to an invalid URL via location.href with process-swap-on-navigation enabled, a new WebKitContent process is created.
This causes fast/loader/redirect-to-invalid-url-using-javascript-disallowed.html to timeout with PSON enabled.
Comment 1 Radar WebKit Bug Importer 2018-04-26 21:39:49 PDT
<rdar://problem/39781795>
Comment 2 Ryosuke Niwa 2018-04-26 21:43:52 PDT
Created attachment 338966 [details]
Fixes the bug
Comment 3 EWS Watchlist 2018-04-26 21:46:43 PDT
Attachment 338966 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1294:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1298:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 EWS Watchlist 2018-04-26 23:29:14 PDT
Comment on attachment 338966 [details]
Fixes the bug

Attachment 338966 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7477296

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 5 EWS Watchlist 2018-04-26 23:29:26 PDT
Created attachment 338977 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 6 youenn fablet 2018-04-27 14:04:11 PDT
Comment on attachment 338966 [details]
Fixes the bug

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

> Source/WebKit/UIProcess/WebProcessPool.cpp:2099
> +    if (!url.isValid() || !targetURL.isValid() || url.isEmpty() || url.isBlankURL() || protocolHostAndPortAreEqual(url, targetURL))

Since the check is related to targetURL, it might be clearer to update shouldTreatAsSameOriginNavigation so that navigation.treatAsSameOriginNavigation() handles this case.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:62
> +bool didRecieveAlert;

s/didRecieveAlert/didReceiveAlert/

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:145
> +    alertMessage = message;

It seems alertMessage is never used.
Are you planning to use it later on or should it be removed?
Comment 7 Chris Dumez 2018-04-27 14:22:28 PDT
Comment on attachment 338966 [details]
Fixes the bug

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:146
> +    didRecieveAlert = true;

Typo here too.
Comment 8 Ryosuke Niwa 2018-04-27 14:25:04 PDT
(In reply to youenn fablet from comment #6)
> Comment on attachment 338966 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338966&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:2099
> > +    if (!url.isValid() || !targetURL.isValid() || url.isEmpty() || url.isBlankURL() || protocolHostAndPortAreEqual(url, targetURL))
> 
> Since the check is related to targetURL, it might be clearer to update
> shouldTreatAsSameOriginNavigation so that
> navigation.treatAsSameOriginNavigation() handles this case.

Brady wanted to check it in UIProcess side so I'm keeping it here.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:62
> > +bool didRecieveAlert;
> 
> s/didRecieveAlert/didReceiveAlert/

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:145
> > +    alertMessage = message;
> 
> It seems alertMessage is never used.
> Are you planning to use it later on or should it be removed?

Removed.

(In reply to Chris Dumez from comment #7)
> Comment on attachment 338966 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338966&action=review
> 
> > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:146
> > +    didRecieveAlert = true;
> 
> Typo here too.

Clearly, I was just copying & pasting LOL. Fixed both.
Comment 9 Ryosuke Niwa 2018-04-27 14:31:21 PDT
Created attachment 339030 [details]
Patch for landing
Comment 10 EWS Watchlist 2018-04-27 14:49:55 PDT
Attachment 339030 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1292:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1296:  Multi-line string ("...") found.  This lint script doesn't do well with such strings, and may give bogus warnings.  They're ugly and unnecessary, and you should use concatenation instead".  [readability/multiline_string] [5]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Ryosuke Niwa 2018-04-27 15:30:45 PDT
Committed r231115: <https://trac.webkit.org/changeset/231115>