Bug 184962 - PSON: Don't create a new process when navigating to a blob URL, data URL, and about:blank
Summary: PSON: Don't create a new process when navigating to a blob URL, data URL, and...
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-25 02:09 PDT by Ryosuke Niwa
Modified: 2018-04-25 16:17 PDT (History)
7 users (show)

See Also:


Attachments
Fixes the bug (17.75 KB, patch)
2018-04-25 02:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Made data: and about: use same process (21.44 KB, patch)
2018-04-25 11:58 PDT, Ryosuke Niwa
youennf: review+
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-25 02:09:54 PDT
Don't create a new WebContent process when navigating to a blob object URL
if the source document has access to it.
Comment 1 Radar WebKit Bug Importer 2018-04-25 02:10:24 PDT
<rdar://problem/39715044>
Comment 2 Ryosuke Niwa 2018-04-25 02:21:36 PDT
Created attachment 338716 [details]
Fixes the bug
Comment 3 EWS Watchlist 2018-04-25 02:22:54 PDT
Attachment 338716 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1057:  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:1063:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1066:  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: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 youenn fablet 2018-04-25 09:44:30 PDT
Comment on attachment 338716 [details]
Fixes the bug

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

> Source/WebCore/loader/NavigationAction.cpp:60
> +    , m_isSameOriginBlobNavigation { isSameOriginBlogNavigation(source, resourceRequest.url()) }

Should we have the same strategy with data URLs for performance reasons?
Should we add a m_isConsideredAsSameOriginNavigation boolean flag taking care of all these cases (blob URLs, data URLs, about:blank, same origin URLs, file URLs...) ?
Comment 5 Ryosuke Niwa 2018-04-25 11:15:16 PDT
(In reply to youenn fablet from comment #4)
> Comment on attachment 338716 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338716&action=review
> 
> > Source/WebCore/loader/NavigationAction.cpp:60
> > +    , m_isSameOriginBlobNavigation { isSameOriginBlogNavigation(source, resourceRequest.url()) }
> 
> Should we have the same strategy with data URLs for performance reasons?
> Should we add a m_isConsideredAsSameOriginNavigation boolean flag taking
> care of all these cases (blob URLs, data URLs, about:blank, same origin
> URLs, file URLs...) ?

That probably makes sense. Maybe m_shouldTreatAsSameOriginNavigation.
Comment 6 Ryosuke Niwa 2018-04-25 11:58:35 PDT
Created attachment 338766 [details]
Made data: and about: use same process
Comment 7 EWS Watchlist 2018-04-25 12:01:43 PDT
Attachment 338766 [details] did not pass style-queue:


ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1057:  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:1063:  Missing spaces around /  [whitespace/operators] [3]
ERROR: Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:1066:  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: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 youenn fablet 2018-04-25 13:18:16 PDT
Comment on attachment 338766 [details]
Made data: and about: use same process

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

> Source/WebCore/ChangeLog:9
> +        to avoid creating a new WebContent process when navigating to a blob object URL.

s/blob object//

> Source/WebKit/UIProcess/WebPageProxy.cpp:3959
> +    navigation->settreatAsSameOriginNavigation(navigationActionData.treatAsSameOriginNavigation);

s/settreatAsSameOriginNavigation/setTreatAsSameOriginNavigation/

> Source/WebKit/UIProcess/WebProcessPool.cpp:2027
> +    if (navigation.treatAsSameOriginNavigation())

Can we simplify the logic here with the lines below checking for !url.isValid() || url.isEmpty() || url.isBlankURL() || protocolHostAndPortAreEqual(url, targetURL)? Maybe WebProcess could do the computation here.
I am wondering what we should do with file based origins/loads as well.

> Source/WebKit/UIProcess/API/APINavigation.h:87
> +    void settreatAsSameOriginNavigation(bool value) { m_treatAsSameOriginNavigation = value; }

s/sett/setT/
Comment 9 Ryosuke Niwa 2018-04-25 13:38:47 PDT
Comment on attachment 338766 [details]
Made data: and about: use same process

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

>> Source/WebKit/UIProcess/WebProcessPool.cpp:2027
>> +    if (navigation.treatAsSameOriginNavigation())
> 
> Can we simplify the logic here with the lines below checking for !url.isValid() || url.isEmpty() || url.isBlankURL() || protocolHostAndPortAreEqual(url, targetURL)? Maybe WebProcess could do the computation here.
> I am wondering what we should do with file based origins/loads as well.

Okay, I talked with Brady. We can't move protocolHostAndPortAreEqual because of redirects but we can move isEmpty() and isBlankURL() so let's do that.
Comment 10 Ryosuke Niwa 2018-04-25 13:48:05 PDT
Committed r231019: <https://trac.webkit.org/changeset/231019>
Comment 11 Ryosuke Niwa 2018-04-25 14:28:42 PDT
(In reply to Ryosuke Niwa from comment #9)
> Comment on attachment 338766 [details]
> Made data: and about: use same process
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338766&action=review
> 
> >> Source/WebKit/UIProcess/WebProcessPool.cpp:2027
> >> +    if (navigation.treatAsSameOriginNavigation())
> > 
> > Can we simplify the logic here with the lines below checking for !url.isValid() || url.isEmpty() || url.isBlankURL() || protocolHostAndPortAreEqual(url, targetURL)? Maybe WebProcess could do the computation here.
> > I am wondering what we should do with file based origins/loads as well.
> 
> Okay, I talked with Brady. We can't move protocolHostAndPortAreEqual because
> of redirects but we can move isEmpty() and isBlankURL() so let's do that.

Actually, this breaks layout tests. We need to keep isBlankURL() and isEmpty() check here.
Comment 12 Ryosuke Niwa 2018-04-25 16:13:22 PDT
It's because the check in WebProcessPool::processForNavigation is about the originating / source URL, not the target URL.
Comment 13 Ryosuke Niwa 2018-04-25 16:17:33 PDT
Committed r231031: <https://trac.webkit.org/changeset/231031>