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

Issue 818875 link

Starred by 1 user
Status: Fixed
Owner:
karandeepb@chromium.org
Closed: Mar 2018
Cc:
jam@chromium.org
█ roc...@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Network Servicification: Undefined behavior in RegisterNonNetworkSubresourceURLLoaderFactories.

Project Member Reported by karandeepb@chromium.org, Mar 5 2018 
In ChromeContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories, on adding a DCHECK as follows:

extensions::ChromeExtensionWebContentsObserver* web_observer =
      extensions::ChromeExtensionWebContentsObserver::FromWebContents(
          web_contents);
DCHECK(web_observer);

and running  

out/Default/browser_tests --gtest_filter="TabCaptureApiPixelTest.OffscreenTabEndToEnd" --enable-features=NetworkService

the test crashes at the DCHECK. The WebContentsObserver can be null here. This is also true for some other tests.

Comment 1 by karandeepb@chromium.org, Mar 5 2018

Cc: roc...@chromium.org jam@chromium.org
Owner: karandeepb@chromium.org
Status: Assigned (was: Untriaged)
This is also true for at least these tests:

ConstrainedWebDialogBrowserTest.ContentResizeInAutoResizingDialog
ConstrainedWebDialogBrowserTest.ContentResizeInNonAutoResizingDialog
ProfileChooserViewExtensionsTest.LockProfile
ProfileChooserViewExtensionsTest.LockProfileBlockExtensions
ProfileChooserViewExtensionsTest.LockProfileNoBlockOtherProfileExtensions
TabCaptureApiPixelTest.OffscreenTabEndToEnd
TabCaptureApiPixelTest.OffscreenTabEvilTests
TabCaptureApiTest.MaxOffscreenTabs

rockot@, jam@: I can go ahead and disable these tests in the filter file. Also the tests still pass if we return early if |web_observer| is null (But I am not sure if that's the correct thing to do). This came up while investigating trybot failures for https://chromium-review.googlesource.com/c/chromium/src/+/940863 and this is holding it being landed.

Comment 2 by roc...@chromium.org, Mar 5 2018 

I think we should just replace the DCHECK with an early return. We can't do anything if there's no observer so it shouldn't matter. Should be sufficient to have a comment explaining that some test environments do not create said observer.
Project Member

Comment 3 by bugdroid1@chromium.org, Mar 6 2018 

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

commit 919285763bcd814bdbf49e1a808f90fb6bf9b9cd
Author: Karan Bhatia <karandeepb@chromium.org>
Date: Tue Mar 06 01:18:07 2018

NetworkService: Fix undefined behavior in RegisterNonNetworkSubresourceURLLoaderFactories.

In ChromeContentBrowserClient::RegisterNonNetworkSubresourceURLLoaderFactories,
it's possible that no ExtensionWebContentsObserver is attached to the
WebContents for the given RenderFrameHost. Return early in that case.

BUG= 818875 

Change-Id: Ifca9cc67d36fe5aee0da496952125ef8ee199849
Reviewed-on: https://chromium-review.googlesource.com/950134
Reviewed-by: Ken Rockot <rockot@chromium.org>
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#541014}
[modify] https://crrev.com/919285763bcd814bdbf49e1a808f90fb6bf9b9cd/chrome/browser/chrome_content_browser_client.cc

Comment 4 by karandeepb@chromium.org, Mar 6 2018

Status: Fixed (was: Assigned)
►

Sign in to add a comment