New issue
Advanced search Search tips

Issue 702374 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug



Sign in to add a comment

Extension Test failures on Stable due to r452240

Project Member Reported by karandeepb@chromium.org, Mar 16 2017

Issue description

r452240 disabled creation of Web Request rules registry on the Stable channel. This is leading to multiple test failures on Stable, see https://codereview.chromium.org/2753073003/
 
Summary: Extension Test failures on Stable due to r452240 (was: Test failures on Stable due to r452240)
Cc: rdevlin....@chromium.org
Status: Fixed (was: Assigned)
Devlin: It seems to me there won't be any actual test failures. See https://codereview.chromium.org/2756133003/. All/most of the tests run with Dev as the current channel. With the change in https://codereview.chromium.org/2753073003/, the tests were failing due to a channel mismatch. 

Aside: It seems we should extend our test coverage to test on other channels besides Dev.
Cc: lazyboy@chromium.org
Hmm... I'm not quite convinced.

https://codereview.chromium.org/2756133003/ changes this line:
  // Only set if it hasn't already been set (e.g. by a test).
  if (GetCurrentChannel() == GetDefaultChannel())
    SetCurrentChannel(chrome::GetChannel());

to this
  // Only set if it hasn't already been set (e.g. by a test).
  if (GetCurrentChannel() == GetDefaultChannel())
    SetCurrentChannel(version_info::Channel::STABLE);

But that comment is important - if the current channel is already set, e.g. by a test, it won't set the channel to stable.  And we do just that in ExtensionBrowserTest [1].

You can verify this by adding some logs to ChromeExtensionsBrowserClient - you'll see that the if is never entered, and that the init work happens in extensions/browser/api/declarative/rules_registry_service.cc.

A few notes:
- An easy way to test this is just to substitute the change from https://codereview.chromium.org/2753073003/ with "false" in the availability if.  There's still a concern about the mismatch of assumptions, but it would get the fundamental state.
- What made me think of this is that we apparently allow <webview> to use declarativeWebRequest APIs, and as far as I know, we allow it on stable.

[1] https://chromium.googlesource.com/chromium/src/+/2b588924dbe2205d5f52f4cb453d8d2dc20afb0b/chrome/browser/extensions/extension_browsertest.h#375
@comment 3, yes <webview> shipped declarative webrequest on stable a while ago.
Status: Assigned (was: Fixed)
Cc: vabr@chromium.org
>>Hmm... I'm not quite convinced.
>>
>>https://codereview.chromium.org/2756133003/ changes this line:
>>  // Only set if it hasn't already been set (e.g. by a test).
>>  if (GetCurrentChannel() == GetDefaultChannel())
>>    SetCurrentChannel(chrome::GetChannel());
>>
>>to this
>>  // Only set if it hasn't already been set (e.g. by a test).
>>  if (GetCurrentChannel() == GetDefaultChannel())
>>    SetCurrentChannel(version_info::Channel::STABLE);
>>
>>But that comment is important - if the current channel is already set, e.g. by a test, it won't set the channel to stable.  And we do just that in ExtensionBrowserTest [1].
>>
>>You can verify this by adding some logs to ChromeExtensionsBrowserClient - you'll see that the if is never entered, and that the init work happens in extensions/browser/api/declarative/rules_registry_service.cc.

Yeah I was trying to showcase that extension related tests won't fail on the Stable builder but the CL is incorrect since webview uses DWR on stable. 

Looping in vabr@ who initially enabled the block in r183467 which is causing the delay.

vabr@: Can the block you had enabled in https://chromiumcodereview.appspot.com/12230017 simply be reverted? This can cause significant delay even on Stable - https://uma.googleplex.com/p/chrome/histograms/?endDate=20170319&dayCount=28&histograms=Extensions.NetworkDelayRegistryLoad&fixupData=true&showMax=true&filters=channel%2Ceq%2C4%2Cisofficial%2Ceq%2CTrue&implicitFilters=isofficial

Else I'll just revert my change (r452240) which disabled rules registry loading on Stable.

Comment 7 by vabr@chromium.org, Mar 23 2017

If Chrome stops blocking network requests until the rules registry is up, some extensions on Beta, Dev and Canary might stop working properly on startup, letting some requests go unnoticed. Because such extensions are often related to privacy, I suggest that you check with the Chrome Privacy team whether that would be OK.

Other than that, I have no concerns (with the caveat that it's been 4+ years since I made that change and I don't remember much about WebRequest since then).
>> If Chrome stops blocking network requests until the rules registry is up, some extensions on Beta, Dev and Canary might stop working properly on startup, letting some requests go unnoticed.

Actually as c#4 pointed out, Declarative Web Request is also enabled on Stable through webviews. Since not loading the rules registry would affect the behavior of webviews using DWR on startup, I'll simply revert my change.

Status: Fixed (was: Assigned)
r452240 was reverted in r459269. Tracking merge of the revert to M58 in  issue 693243 .
Cc: -vabr@chromium.org

Sign in to add a comment