Bug 184962 – PSON: Don't create a new process when navigating to a blob URL, data URL, and about:blank

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Don't create a new WebContent process when navigating to a blob object URL
if the source document has access to it.

[Radar WebKit Bug Importer]

<rdar://problem/39715044>

[Ryosuke Niwa]

Created attachment 338716 [details]
Fixes the bug

[EWS Watchlist]

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.

[youenn fablet]

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...) ?

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Created attachment 338766 [details]
Made data: and about: use same process

[EWS Watchlist]

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.

[youenn fablet]

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/

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Committed r231019: <https://trac.webkit.org/changeset/231019>

[Ryosuke Niwa]

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

[Ryosuke Niwa]

It's because the check in WebProcessPool::processForNavigation is about the originating / source URL, not the target URL.

[Ryosuke Niwa]

Committed r231031: <https://trac.webkit.org/changeset/231031>