New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 700514 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Out until 24 Jan
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

SiteProcessMap::RegisterProcess can overwrite existing entries

Project Member Reported by nasko@chromium.org, Mar 10 2017

Issue description

The current implementation of SiteProcessMap::RegisterProcess does not check if the site being registered is already present in the map. This can allow processes in some cases to overwrite the entry in the table and become the process hosting the site in the process-per-site process model.
 

Comment 1 by nasko@chromium.org, Mar 11 2017

Cc: rdevlin....@chromium.org
This bug was found as part of investigating why the ExtensionWebRequestApiTest.WebRequestBlocking test is failing with the new error page commit policy. It revealed the following sequence of events, which caused the overwrite to occur.

1. Tab A is navigated to chrome-extension://foo/test.html

2. The navigation in step 1 creates a process for the extension - P1. This registers the chrome-extension://foo/ URL in the SiteProcessMap with P1 as the process. From this point on, any SiteInstances for chrome-extension://foo/ should be using P1.

3. Tab A opens tab B using the chrome.tabs.create() API and supplies "about:blank" as the URL to navigate to. Since it is an unrelated tab, it gets its own BrowsingInstance, SiteInstance, and process - P2. Since the navigation is to "about:blank", the SiteInstance remains without assigned Site URL.

4. Tab A navigates tab B to chrome-extension://foo/blocked.html, which is an URL that the same extension blocks through webRequest extension API. The blocking results in ERR_BLOCKED_BY_CLIENT error, which in turn means the browser will display an error page.

5. The error page is then committed in P2 and sends a DidCommitProvisionalLoad IPC to the browser process.

6. The browser process looks at the SiteInstance for tab 2, notices it hasn't been assigned any site yet, so it uses the committed URL to set it to. However, error pages send the destination URL as the URL in the IPC message, which means the SiteInstance is set to chrome-extension://foo/ URL.

7. Internally, SiteInstance registers the site and process in the SiteProcessMap.

8. Because there is no check for existing entry, the SiteProcessMap overwrites the entry for chrome-extension://foo/ to point to P2, which is not even an extension process.
Project Member

Comment 2 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0e9d619f88ffabc3c237cbdf75603df8af9925e6

commit 0e9d619f88ffabc3c237cbdf75603df8af9925e6
Author: nasko <nasko@chromium.org>
Date: Wed Mar 15 00:42:58 2017

Check for already existing entry when adding to SiteProcessMap.

The SiteProcessMap::RegisterProcess can overwrite existing entries
in the map, which can lead to incorrect process being mapped to
a site in process-per-site mode. This CL checks for existence and
avoids the overwrite.

BUG= 700514 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2739193004
Cr-Commit-Position: refs/heads/master@{#456920}

[modify] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/chrome/browser/extensions/process_management_browsertest.cc
[add] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/chrome/test/data/extensions/web_request_site_process_registration/blocked.html
[add] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/chrome/test/data/extensions/web_request_site_process_registration/manifest.json
[add] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/chrome/test/data/extensions/web_request_site_process_registration/test.html
[add] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/chrome/test/data/extensions/web_request_site_process_registration/test.js
[modify] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/0e9d619f88ffabc3c237cbdf75603df8af9925e6/content/browser/renderer_host/render_process_host_impl.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96bb282f55edfa739e4e5f6633bb5fce61583e87

commit 96bb282f55edfa739e4e5f6633bb5fce61583e87
Author: tzik <tzik@chromium.org>
Date: Wed Mar 15 06:19:46 2017

Revert of Check for already existing entry when adding to SiteProcessMap. (patchset #7 id:160001 of https://codereview.chromium.org/2739193004/ )

Reason for revert:
Speculative revert to see a bot failure.

SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe got flaky around this CL, and this is relatively more suspicious in the failing range.

BUG= 701663 

Original issue's description:
> Check for already existing entry when adding to SiteProcessMap.
>
> The SiteProcessMap::RegisterProcess can overwrite existing entries
> in the map, which can lead to incorrect process being mapped to
> a site in process-per-site mode. This CL checks for existence and
> avoids the overwrite.
>
> BUG= 700514 
> CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
>
> Review-Url: https://codereview.chromium.org/2739193004
> Cr-Commit-Position: refs/heads/master@{#456920}
> Committed: https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf75603df8af9925e6

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,nasko@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 700514 

Review-Url: https://codereview.chromium.org/2749543004
Cr-Commit-Position: refs/heads/master@{#457010}

[modify] https://crrev.com/96bb282f55edfa739e4e5f6633bb5fce61583e87/chrome/browser/extensions/process_management_browsertest.cc
[delete] https://crrev.com/f9d855c6039ca2daddbd4c3d08caac142fd15966/chrome/test/data/extensions/web_request_site_process_registration/blocked.html
[delete] https://crrev.com/f9d855c6039ca2daddbd4c3d08caac142fd15966/chrome/test/data/extensions/web_request_site_process_registration/manifest.json
[delete] https://crrev.com/f9d855c6039ca2daddbd4c3d08caac142fd15966/chrome/test/data/extensions/web_request_site_process_registration/test.html
[delete] https://crrev.com/f9d855c6039ca2daddbd4c3d08caac142fd15966/chrome/test/data/extensions/web_request_site_process_registration/test.js
[modify] https://crrev.com/96bb282f55edfa739e4e5f6633bb5fce61583e87/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/96bb282f55edfa739e4e5f6633bb5fce61583e87/content/browser/renderer_host/render_process_host_impl.cc

Project Member

Comment 4 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0bcbed7d67f9a2813e1a278aada6c047a26bbbff

commit 0bcbed7d67f9a2813e1a278aada6c047a26bbbff
Author: nasko <nasko@chromium.org>
Date: Wed Mar 15 17:03:35 2017

Reland of Check for already existing entry when adding to SiteProcessMap. (patchset #1 id:1 of https://codereview.chromium.org/2749543004/ )

Reason for revert:
Reverting the revert to see if this was indeed the culprit for the flakiness.

Original issue's description:
> Revert of Check for already existing entry when adding to SiteProcessMap. (patchset #7 id:160001 of https://codereview.chromium.org/2739193004/ )
>
> Reason for revert:
> Speculative revert to see a bot failure.
>
> SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe got flaky around this CL, and this is relatively more suspicious in the failing range.
>
> BUG= 701663 
>
>
> Original issue's description:
> > Check for already existing entry when adding to SiteProcessMap.
> >
> > The SiteProcessMap::RegisterProcess can overwrite existing entries
> > in the map, which can lead to incorrect process being mapped to
> > a site in process-per-site mode. This CL checks for existence and
> > avoids the overwrite.
> >
> > BUG= 700514 
> > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
> >
> > Review-Url: https://codereview.chromium.org/2739193004
> > Cr-Commit-Position: refs/heads/master@{#456920}
> > Committed: https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf75603df8af9925e6
>
> TBR=creis@chromium.org,rdevlin.cronin@chromium.org,nasko@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 700514 
>
> Review-Url: https://codereview.chromium.org/2749543004
> Cr-Commit-Position: refs/heads/master@{#457010}
> Committed: https://chromium.googlesource.com/chromium/src/+/96bb282f55edfa739e4e5f6633bb5fce61583e87

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,tzik@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 701663 

Review-Url: https://codereview.chromium.org/2755643003
Cr-Commit-Position: refs/heads/master@{#457110}

[modify] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/chrome/browser/extensions/process_management_browsertest.cc
[add] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/chrome/test/data/extensions/web_request_site_process_registration/blocked.html
[add] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/chrome/test/data/extensions/web_request_site_process_registration/manifest.json
[add] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/chrome/test/data/extensions/web_request_site_process_registration/test.html
[add] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/chrome/test/data/extensions/web_request_site_process_registration/test.js
[modify] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/0bcbed7d67f9a2813e1a278aada6c047a26bbbff/content/browser/renderer_host/render_process_host_impl.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Mar 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7542ac912821eb3d5c939f17de433621d60f7fab

commit 7542ac912821eb3d5c939f17de433621d60f7fab
Author: nasko <nasko@chromium.org>
Date: Wed Mar 15 21:20:00 2017

Revert of Check for already existing entry when adding to SiteProcessMap. (patchset #1 id:1 of https://codereview.chromium.org/2755643003/ )

Reason for revert:
Test is flaking again, reverting the reland.

Original issue's description:
> Reland of Check for already existing entry when adding to SiteProcessMap. (patchset #1 id:1 of https://codereview.chromium.org/2749543004/ )
>
> Reason for revert:
> Reverting the revert to see if this was indeed the culprit for the flakiness.
>
> Original issue's description:
> > Revert of Check for already existing entry when adding to SiteProcessMap. (patchset #7 id:160001 of https://codereview.chromium.org/2739193004/ )
> >
> > Reason for revert:
> > Speculative revert to see a bot failure.
> >
> > SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe got flaky around this CL, and this is relatively more suspicious in the failing range.
> >
> > BUG= 701663 
> >
> >
> > Original issue's description:
> > > Check for already existing entry when adding to SiteProcessMap.
> > >
> > > The SiteProcessMap::RegisterProcess can overwrite existing entries
> > > in the map, which can lead to incorrect process being mapped to
> > > a site in process-per-site mode. This CL checks for existence and
> > > avoids the overwrite.
> > >
> > > BUG= 700514 
> > > CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
> > >
> > > Review-Url: https://codereview.chromium.org/2739193004
> > > Cr-Commit-Position: refs/heads/master@{#456920}
> > > Committed: https://chromium.googlesource.com/chromium/src/+/0e9d619f88ffabc3c237cbdf75603df8af9925e6
> >
> > TBR=creis@chromium.org,rdevlin.cronin@chromium.org,nasko@chromium.org
> > # Skipping CQ checks because original CL landed less than 1 days ago.
> > NOPRESUBMIT=true
> > NOTREECHECKS=true
> > NOTRY=true
> > BUG= 700514 
> >
> > Review-Url: https://codereview.chromium.org/2749543004
> > Cr-Commit-Position: refs/heads/master@{#457010}
> > Committed: https://chromium.googlesource.com/chromium/src/+/96bb282f55edfa739e4e5f6633bb5fce61583e87
>
> TBR=creis@chromium.org,rdevlin.cronin@chromium.org,tzik@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 701663 
>
> Review-Url: https://codereview.chromium.org/2755643003
> Cr-Commit-Position: refs/heads/master@{#457110}
> Committed: https://chromium.googlesource.com/chromium/src/+/0bcbed7d67f9a2813e1a278aada6c047a26bbbff

TBR=creis@chromium.org,rdevlin.cronin@chromium.org,tzik@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 701663 

Review-Url: https://codereview.chromium.org/2756573002
Cr-Commit-Position: refs/heads/master@{#457211}

[modify] https://crrev.com/7542ac912821eb3d5c939f17de433621d60f7fab/chrome/browser/extensions/process_management_browsertest.cc
[delete] https://crrev.com/3e36a03e908bfc7bf9a4335611ae4789280c1506/chrome/test/data/extensions/web_request_site_process_registration/blocked.html
[delete] https://crrev.com/3e36a03e908bfc7bf9a4335611ae4789280c1506/chrome/test/data/extensions/web_request_site_process_registration/manifest.json
[delete] https://crrev.com/3e36a03e908bfc7bf9a4335611ae4789280c1506/chrome/test/data/extensions/web_request_site_process_registration/test.html
[delete] https://crrev.com/3e36a03e908bfc7bf9a4335611ae4789280c1506/chrome/test/data/extensions/web_request_site_process_registration/test.js
[modify] https://crrev.com/7542ac912821eb3d5c939f17de433621d60f7fab/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/7542ac912821eb3d5c939f17de433621d60f7fab/content/browser/renderer_host/render_process_host_impl.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Mar 17 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0485e8ec0ab7d527744dcd61328b7f23a1eecfda

commit 0485e8ec0ab7d527744dcd61328b7f23a1eecfda
Author: engedy <engedy@chromium.org>
Date: Fri Mar 17 16:48:58 2017

Make SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe non-flaky.

When a provisional load fails with no server-supplied error page, Chrome's own
navigation error page is shown. This also triggers a background request to load
navigation corrections (aka. Link Doctor). Once the results are back, there is
a navigation to a second error page with the suggestions.

Thes CL makes the SubresourceFilterBrowserTest.FailedProvisionalLoadInMainframe
expect and wait for this second navigation. Otherwise the completion of loading
the Link Doctor error page would be at race with that of subsequent navigations
initiated in the test, leading to flakiness.

BUG=637415, 700514 

Review-Url: https://codereview.chromium.org/2754173003
Cr-Commit-Position: refs/heads/master@{#457796}

[modify] https://crrev.com/0485e8ec0ab7d527744dcd61328b7f23a1eecfda/chrome/browser/subresource_filter/subresource_filter_browsertest.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Mar 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1aaa76016c981807fbe10c53c00de6061200c98

commit d1aaa76016c981807fbe10c53c00de6061200c98
Author: nasko <nasko@chromium.org>
Date: Sat Mar 18 02:14:45 2017

Check for already existing entry when adding to SiteProcessMap.

The SiteProcessMap::RegisterProcess can overwrite existing entries
in the map, which can lead to incorrect process being mapped to
a site in process-per-site mode. This CL checks for existence and
avoids the overwrite.

This CL is a reland of https://codereview.chromium.org/2739193004/
after the flaky test was fixed in https://codereview.chromium.org/2754173003/

BUG= 700514 
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Review-Url: https://codereview.chromium.org/2760733002
Cr-Commit-Position: refs/heads/master@{#457925}

[modify] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/browser/extensions/process_management_browsertest.cc
[modify] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/test/data/extensions/api_test/webrequest/test_blocking.js
[add] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/test/data/extensions/web_request_site_process_registration/blocked.html
[add] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/test/data/extensions/web_request_site_process_registration/manifest.json
[add] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/test/data/extensions/web_request_site_process_registration/test.html
[add] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/chrome/test/data/extensions/web_request_site_process_registration/test.js
[modify] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/content/browser/frame_host/navigator_impl.cc
[modify] https://crrev.com/d1aaa76016c981807fbe10c53c00de6061200c98/content/browser/renderer_host/render_process_host_impl.cc

Comment 8 by nasko@chromium.org, Mar 20 2017

Status: Fixed (was: Assigned)
It looks like the fix is sticking now, so resolving as fixed.

Sign in to add a comment