Extension Test failures on Stable due to r452240 |
|||||||
Issue descriptionr452240 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/
,
Mar 20 2017
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.
,
Mar 20 2017
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
,
Mar 20 2017
@comment 3, yes <webview> shipped declarative webrequest on stable a while ago.
,
Mar 21 2017
,
Mar 22 2017
>>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.
,
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).
,
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. 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.
,
Mar 28 2017
r452240 was reverted in r459269. Tracking merge of the revert to M58 in issue 693243 .
,
Nov 29
|
|||||||
►
Sign in to add a comment |
|||||||
Comment 1 by karandeepb@chromium.org
, Mar 16 2017