New issue
Advanced search Search tips

Issue 884006 link

Starred by 3 users
Status: Fixed
Owner:
cduvall@chromium.org
Closed: Sep 20
Cc:
jam@chromium.org
cduvall@chromium.org
rhalavati@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Mac
Pri: 2
Type: Bug



Sign in to add a comment

[Network service] Need to wait for content scripts to load before navigation.

Project Member Reported by cduvall@chromium.org, Sep 13 
See background in issue 879883, in non-NS path ChromeResourceDispatcherHostDelegate uses UserScriptListener::CreateResourceThrottle to block requests until content scripts have been loaded from extensions. This is not implemented with NS enabled.

Comment 1 by cduvall@chromium.org, Sep 14

Owner: cduvall@chromium.org
Status: Started (was: Untriaged)

Comment 2 by dxie@chromium.org, Sep 18

Labels: Hotlist-KnownIssue
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 18 

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

commit 888677bc9b7351f2dd999d3632d718cf6312140f
Author: Clark DuVall <cduvall@chromium.org>
Date: Tue Sep 18 20:44:34 2018

Switch UserScriptListener ResourceThrottle to NavigationThrottle

ResourceThrottles do not work with network service enabled, a
NavigationThrottle will work both with NS enabled and disabled.

Bug:  884006 
Change-Id: Ifea0e59bd67d70b593b46c2a2a2bd4be0e3f98f0
Reviewed-on: https://chromium-review.googlesource.com/1227358
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592182}
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/chrome_extensions_browser_client.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/chrome_extensions_browser_client.h
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/user_script_listener.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/user_script_listener.h
[add] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/user_script_listener_browsertest.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/extensions/user_script_listener_unittest.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/chrome/test/BUILD.gn
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/content/public/browser/navigation_handle.h
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/extensions/browser/extensions_browser_client.cc
[modify] https://crrev.com/888677bc9b7351f2dd999d3632d718cf6312140f/extensions/browser/extensions_browser_client.h
Project Member

Comment 4 by bugdroid1@chromium.org, Sep 19 

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

commit 18f93e3423700b1faa224a382c90a6b5a4aaf841
Author: Asanka Herath <asanka@chromium.org>
Date: Wed Sep 19 16:17:17 2018

Revert "Switch UserScriptListener ResourceThrottle to NavigationThrottle"

This reverts commit 888677bc9b7351f2dd999d3632d718cf6312140f.

Reason for revert: Tests failing on chromium.memory/Linux CFI. See  crbug.com/886576 

Original change's description:
> Switch UserScriptListener ResourceThrottle to NavigationThrottle
> 
> ResourceThrottles do not work with network service enabled, a
> NavigationThrottle will work both with NS enabled and disabled.
> 
> Bug:  884006 
> Change-Id: Ifea0e59bd67d70b593b46c2a2a2bd4be0e3f98f0
> Reviewed-on: https://chromium-review.googlesource.com/1227358
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#592182}

TBR=jam@chromium.org,rdevlin.cronin@chromium.org,cduvall@chromium.org

Change-Id: I18519542954bf23e2e76ea83598382f4f00188e3
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  884006 
Reviewed-on: https://chromium-review.googlesource.com/1234214
Reviewed-by: Asanka Herath <asanka@chromium.org>
Commit-Queue: Asanka Herath <asanka@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592428}
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/extensions/chrome_extensions_browser_client.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/extensions/chrome_extensions_browser_client.h
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/extensions/user_script_listener.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/extensions/user_script_listener.h
[delete] https://crrev.com/a98c29279ffe393ab635e96d359dbe360de34ae4/chrome/browser/extensions/user_script_listener_browsertest.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/extensions/user_script_listener_unittest.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/chrome/test/BUILD.gn
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/content/public/browser/navigation_handle.h
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/extensions/browser/extensions_browser_client.cc
[modify] https://crrev.com/18f93e3423700b1faa224a382c90a6b5a4aaf841/extensions/browser/extensions_browser_client.h

Comment 5 by asanka@chromium.org, Sep 19

Cc: rhalavati@chromium.org
 Issue 886576  has been merged into this issue.

Comment 6 by asanka@chromium.org, Sep 19 

Merged the issue autofiled for the test failures.
Project Member

Comment 7 by bugdroid1@chromium.org, Sep 19 

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

commit 2cf99249e6d57214d8d183009fd10882bb760870
Author: Clark DuVall <cduvall@chromium.org>
Date: Wed Sep 19 19:11:51 2018

Reland "Switch UserScriptListener ResourceThrottle to NavigationThrottle"

This is a reland of 888677bc9b7351f2dd999d3632d718cf6312140f

Unit tests were failing on Linux CFI bot, needed a couple extra lines
to set up the test WebContents.

Original change's description:
> Switch UserScriptListener ResourceThrottle to NavigationThrottle
>
> ResourceThrottles do not work with network service enabled, a
> NavigationThrottle will work both with NS enabled and disabled.
>
> Bug:  884006 
> Change-Id: Ifea0e59bd67d70b593b46c2a2a2bd4be0e3f98f0
> Reviewed-on: https://chromium-review.googlesource.com/1227358
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#592182}

TBR=jam@chromium.org,rdevlin.cronin@chromium.org

Bug:  884006 
Change-Id: I146dced8038dff7dfed262748602a87dc1993af8
Reviewed-on: https://chromium-review.googlesource.com/1234353
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592502}
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/chrome_extensions_browser_client.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/chrome_extensions_browser_client.h
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/user_script_listener.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/user_script_listener.h
[add] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/user_script_listener_browsertest.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/extensions/user_script_listener_unittest.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/chrome/test/BUILD.gn
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/content/public/browser/navigation_handle.h
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/extensions/browser/extensions_browser_client.cc
[modify] https://crrev.com/2cf99249e6d57214d8d183009fd10882bb760870/extensions/browser/extensions_browser_client.h

Comment 8 by cduvall@chromium.org, Sep 19

Labels: Merge-Request-70 M-70
Requesting a merge to M70 for change in #7, the change in #3 has been in canary, and there are just unit test changes needed for the reland.

This change fixes a regression in content script loading in network service that will be good to have when we enable on Beta.
Project Member

Comment 9 by sheriffbot@chromium.org, Sep 19

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10 by benmason@chromium.org, Sep 20

Labels: -Hotlist-Merge-Review -Merge-Review-70 Merge-Approved-70
Approved for merge to 70, branch 3538.
Project Member

Comment 11 by cr-audit...@appspot.gserviceaccount.com, Sep 20

Labels: -Merge-Approved-70 Merge-Merged-70-refsbranch-heads3538
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8ed93f984c754f51f3e752deeea68e9053ad38dc
Commit: 8ed93f984c754f51f3e752deeea68e9053ad38dc
Author: cduvall@chromium.org
Commiter: cduvall@chromium.org
Date: 2018-09-20 16:47:12 +0000 UTC
Reland "Switch UserScriptListener ResourceThrottle to NavigationThrottle"

This is a reland of 888677bc9b7351f2dd999d3632d718cf6312140f

Unit tests were failing on Linux CFI bot, needed a couple extra lines
to set up the test WebContents.

Original change's description:
> Switch UserScriptListener ResourceThrottle to NavigationThrottle
>
> ResourceThrottles do not work with network service enabled, a
> NavigationThrottle will work both with NS enabled and disabled.
>
> Bug:  884006 
> Change-Id: Ifea0e59bd67d70b593b46c2a2a2bd4be0e3f98f0
> Reviewed-on: https://chromium-review.googlesource.com/1227358
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#592182}

TBR=cduvall@chromium.org, jam@chromium.org, rdevlin.cronin@chromium.org

(cherry picked from commit 2cf99249e6d57214d8d183009fd10882bb760870)

Bug:  884006 
Change-Id: I146dced8038dff7dfed262748602a87dc1993af8
Reviewed-on: https://chromium-review.googlesource.com/1234353
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592502}
Reviewed-on: https://chromium-review.googlesource.com/1236834
Cr-Commit-Position: refs/branch-heads/3538@{#543}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
Project Member

Comment 12 by bugdroid1@chromium.org, Sep 20

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8ed93f984c754f51f3e752deeea68e9053ad38dc

commit 8ed93f984c754f51f3e752deeea68e9053ad38dc
Author: Clark DuVall <cduvall@chromium.org>
Date: Thu Sep 20 16:47:12 2018

Reland "Switch UserScriptListener ResourceThrottle to NavigationThrottle"

This is a reland of 888677bc9b7351f2dd999d3632d718cf6312140f

Unit tests were failing on Linux CFI bot, needed a couple extra lines
to set up the test WebContents.

Original change's description:
> Switch UserScriptListener ResourceThrottle to NavigationThrottle
>
> ResourceThrottles do not work with network service enabled, a
> NavigationThrottle will work both with NS enabled and disabled.
>
> Bug:  884006 
> Change-Id: Ifea0e59bd67d70b593b46c2a2a2bd4be0e3f98f0
> Reviewed-on: https://chromium-review.googlesource.com/1227358
> Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Commit-Queue: Clark DuVall <cduvall@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#592182}

TBR=cduvall@chromium.org, jam@chromium.org, rdevlin.cronin@chromium.org

(cherry picked from commit 2cf99249e6d57214d8d183009fd10882bb760870)

Bug:  884006 
Change-Id: I146dced8038dff7dfed262748602a87dc1993af8
Reviewed-on: https://chromium-review.googlesource.com/1234353
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Clark DuVall <cduvall@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#592502}
Reviewed-on: https://chromium-review.googlesource.com/1236834
Cr-Commit-Position: refs/branch-heads/3538@{#543}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/chrome_extensions_browser_client.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/chrome_extensions_browser_client.h
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/user_script_listener.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/user_script_listener.h
[add] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/user_script_listener_browsertest.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/extensions/user_script_listener_unittest.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/browser/loader/chrome_resource_dispatcher_host_delegate.h
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/chrome/test/BUILD.gn
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/content/browser/frame_host/navigation_handle_impl.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/content/browser/frame_host/navigation_handle_impl.h
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/content/public/browser/navigation_handle.h
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/extensions/browser/extensions_browser_client.cc
[modify] https://crrev.com/8ed93f984c754f51f3e752deeea68e9053ad38dc/extensions/browser/extensions_browser_client.h

Comment 13 by cduvall@chromium.org, Sep 20

Status: Fixed (was: Started)
►

Sign in to add a comment