|
|
DescriptionFix losing of scoped feature channel in tests when it's equal to stable
This fixes the issue with the value set by ScopedCurrentChannel
potentially being overwritten by the browser process initialization
code.
This could happen when the ScopedCurrentChannel instance was constructed
with the default channel (that is, the STABLE channel) before the browser
initialization code happens, as the latter would then assume that the channel
wasn't set before and overwrite it.
This issue means that it was impossible to set up the STABLE channel in
the browser test in the setup code that runs before the browser initialization,
unless the building environment was configured to be stable.
BUG=none, 626343
TEST=existing tests
Review-Url: https://codereview.chromium.org/2854293003
Cr-Commit-Position: refs/heads/master@{#473402}
Committed: https://chromium.googlesource.com/chromium/src/+/2ae3a0a699cdb174ed7646ba10597f84bbb496ff
Patch Set 1 #
Total comments: 3
Patch Set 2 : Rebase #Patch Set 3 : Remember the set channel also under scoped setter #
Total comments: 6
Patch Set 4 : Take account of override count #
Messages
Total messages: 31 (20 generated)
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
emaxx@chromium.org changed reviewers: + lazyboy@chromium.org
lazyboy@: PTAL.
lazyboy@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Thanks. I'll forward this to Devlin. He had spent some fun times chasing test failures on bots recently due to Channels and will be able to provide great feedback. https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... extensions/common/features/feature_channel.cc:36: g_current_channel = original_channel_; Q: Two ScoppedCurrentChannel-s only nest, but cannot overlap? Otherwise this will fail: ScopeA ScopeB ~ScopeA ~ScopeB
Just to ensure I understand: > This issue means that it was impossible to set up the STABLE channel in the browser test, unless the building environment was configured to be stable. This isn't quite true, right? If the test created the ScopedCurrentChannel after the ExtensionsBrowserClient was created, wouldn't everything would work correctly?
Description was changed from ========== Fix losing of scoped feature channel when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel), because the browser initialization code would then assume that the channel wasn't set before. This issue means that it was impossible to set up the STABLE channel in the browser test, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests ========== to ========== Fix losing of scoped feature channel when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests ==========
On 2017/05/03 22:43:26, Devlin wrote: > Just to ensure I understand: > > This issue means that it was impossible to set up the STABLE channel in > the browser test, unless the building environment was configured to be > stable. > > This isn't quite true, right? If the test created the ScopedCurrentChannel > after the ExtensionsBrowserClient was created, wouldn't everything would work > correctly? Thanks, that's a good point. Fixed the CL description to be more correct about that. Note that there are already a bunch of tests that actually do create ScopedCurrentChannel before ExtensionsBrowserClient was created, e.g.: * Any test that inherits from ExtensionBrowserTest will create ScopedCurrentChannel(UNKNOWN) in the constructor; * Any test that inherits from ServiceWorkerTest will additionally create ScopedCurrentChannel(STABLE) in the constructor. https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... extensions/common/features/feature_channel.cc:36: g_current_channel = original_channel_; On 2017/05/03 19:01:14, lazyboy wrote: > Q: Two ScoppedCurrentChannel-s only nest, but cannot overlap? Otherwise this > will fail: > ScopeA > ScopeB > ~ScopeA > ~ScopeB Yes, non-nesting overlapping won't work. I don't think it will be easy to support that (also it'll be arguably a weird semantics anyway). Do you think it's worth documenting in the class comment that this case is forbidden?
Description was changed from ========== Fix losing of scoped feature channel when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests ========== to ========== Fix losing of scoped feature channel in tests when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests ==========
rdevlin.cronin@ / lazyboy@: Friendly ping.
https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/1/extensions/common/features/... extensions/common/features/feature_channel.cc:24: if (!g_current_channel_override_count) We'd have this issue already, but I wonder if there's any concern around the flow: ScopedCurrentChannel(BETA); <InitializeSystem> SetCurrentChannel(CANARY); // Ignores; doesn't set. ~ScopedCurrentChannel(); The end result is that the current channel isn't anything we set it to. The way around this would be, conceptually, to have a g_current_channel; optional g_overridden_channel; GetCurrentChannel() { return g_overridden_channel ? *g_overridden_channel : g_current_channel; } But since optionals are non-POD, there's actually a bit more complexity in there. We could solve it with a lazy instance, or a bool did_set, or a counter like you have here. I'm not sure if it's worth addressing that use case, since really we don't *expect* overrides to be appearing and disappearing throughout tests (and if they do, it's probably a bug, since that means some systems would be initialized with certain assumptions, but not others), so I don't know how far we want to take that... just figured I'd bring it up. WDYT?
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/15 20:34:48, Devlin (catching up) wrote: > I'm not sure if it's worth addressing that use case, since really we don't > *expect* overrides to be appearing and disappearing throughout tests (and if > they do, it's probably a bug, since that means some systems would be initialized > with certain assumptions, but not others), so I don't know how far we want to > take that... just figured I'd bring it up. WDYT? I was thinking about this initially, just didn't want to overcomplicate the things. But given that you suggested this too - done.
lgtm, thanks for tolerating the bikeshedding :) https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:14: version_info::Channel g_current_channel = version_info::Channel::STABLE; nit: \n https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:17: version_info::Channel g_overridden_channel = version_info::Channel::STABLE; \n https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:43: << "Scoped channel setters are not nested properly"; One last bit of bikeshedding :) We could actually combine this approach with your previous counter approach, and do something like: int g_override_count = 0; ScopedCurrentChannel::ScopedCurrentChannel(version_info::Channel channel) : channel_(channel), original_overridden_channel_(g_overridden_channel), original_override_count_(g_override_count) { g_overridden_channel = channel; ++g_override_count; } ScopedCurrentChannel::~ScopedCurrentChannel() { DCHECK_EQ(original_override_count_ + 1, g_override_count) << "Scoped channel setters are not nested properly"; g_overridden_channel = original_overridden_channel_; --g_override_count; } That way, we even catch the case of scoped channels not being properly nested when they have the same values (which would be a problem, because the original channel could be different).
The CQ bit was checked by emaxx@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... File extensions/common/features/feature_channel.cc (right): https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:14: version_info::Channel g_current_channel = version_info::Channel::STABLE; On 2017/05/19 22:14:17, Devlin (catching up) wrote: > nit: \n Done. https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:17: version_info::Channel g_overridden_channel = version_info::Channel::STABLE; On 2017/05/19 22:14:17, Devlin (catching up) wrote: > \n Done. https://codereview.chromium.org/2854293003/diff/40001/extensions/common/featu... extensions/common/features/feature_channel.cc:43: << "Scoped channel setters are not nested properly"; On 2017/05/19 22:14:17, Devlin (catching up) wrote: > One last bit of bikeshedding :) > > We could actually combine this approach with your previous counter approach, and > do something like: > > int g_override_count = 0; > > ScopedCurrentChannel::ScopedCurrentChannel(version_info::Channel channel) > : channel_(channel), > original_overridden_channel_(g_overridden_channel), > original_override_count_(g_override_count) { > g_overridden_channel = channel; > ++g_override_count; > } > > ScopedCurrentChannel::~ScopedCurrentChannel() { > DCHECK_EQ(original_override_count_ + 1, g_override_count) > << "Scoped channel setters are not nested properly"; > g_overridden_channel = original_overridden_channel_; > --g_override_count; > } > > That way, we even catch the case of scoped channels not being properly nested > when they have the same values (which would be a problem, because the original > channel could be different). Done.
The CQ bit was checked by emaxx@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2854293003/#ps60001 (title: "Take account of override count")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495248542248300, "parent_rev": "ff559d943fb20ee2c46571979631296c80aed1d6", "commit_rev": "2ae3a0a699cdb174ed7646ba10597f84bbb496ff"}
Message was sent while issue was closed.
Description was changed from ========== Fix losing of scoped feature channel in tests when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests ========== to ========== Fix losing of scoped feature channel in tests when it's equal to stable This fixes the issue with the value set by ScopedCurrentChannel potentially being overwritten by the browser process initialization code. This could happen when the ScopedCurrentChannel instance was constructed with the default channel (that is, the STABLE channel) before the browser initialization code happens, as the latter would then assume that the channel wasn't set before and overwrite it. This issue means that it was impossible to set up the STABLE channel in the browser test in the setup code that runs before the browser initialization, unless the building environment was configured to be stable. BUG=none, 626343 TEST=existing tests Review-Url: https://codereview.chromium.org/2854293003 Cr-Commit-Position: refs/heads/master@{#473402} Committed: https://chromium.googlesource.com/chromium/src/+/2ae3a0a699cdb174ed7646ba1059... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/2ae3a0a699cdb174ed7646ba1059... |