Bug 185066 – PSON: Triggering a navigation to an invalid URL creates a new WebContent process

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

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.

[Radar WebKit Bug Importer]

<rdar://problem/39781795>

[Ryosuke Niwa]

Created attachment 338966 [details]
Fixes the bug

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[youenn fablet]

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?

[Chris Dumez]

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.

[Ryosuke Niwa]

(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.

[Ryosuke Niwa]

Created attachment 339030 [details]
Patch for landing

[EWS Watchlist]

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.

[Ryosuke Niwa]

Committed r231115: <https://trac.webkit.org/changeset/231115>