Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2805033002:
Fix flaky NPE in oobe.js. (Closed)
DescriptionFix flaky NPE in oobe.js.
Under some racy condition, telemetry accesses the DOM object before ready.
This CL adds the variable representing whether DOM gets loaded.
BUG=b/35306355
BUG=709041
TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2805033002
Cr-Commit-Position: refs/heads/master@{#463191}
Committed: https://chromium.googlesource.com/chromium/src/+/d5027325193408828b4dab341dabbc6ff0b7791e
Patch Set 1 #
Total comments: 2
Patch Set 2 : Address review comments #
Total comments: 2
Patch Set 3 : address comments. #Messages
Total messages: 32 (19 generated)
Description was changed from ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. ========== to ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hidehiko@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...
Description was changed from ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 BUG=709041 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + achuith@chromium.org
Corresponding CL is here. https://codereview.chromium.org/2803043002/ PTAL.
nya@chromium.org changed reviewers: + nya@chromium.org
https://codereview.chromium.org/2805033002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_shared.js (right): https://codereview.chromium.org/2805033002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_shared.js:53: Could you define Oobe.readyForTesting here with JSDoc?
Thank you for comment. Done. https://codereview.chromium.org/2805033002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/login/login_shared.js (right): https://codereview.chromium.org/2805033002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/login/login_shared.js:53: On 2017/04/07 04:37:59, Shuhei Takahashi wrote: > Could you define Oobe.readyForTesting here with JSDoc? Done.
The CQ bit was checked by hidehiko@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.
looks good to me.
Thank you. achuithb@, could you take a look as an OWNER? Thank you for review in advance, - hidehiko
Is there an example of where this is a problem? https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_shared.js (right): https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_shared.js:295: Oobe.skipToLoginForTesting = function() { We have a number of testing methods here. Can you move this to down here?
The CQ bit was checked by hidehiko@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...
Thank you for review. > Is there an example of where this is a problem? Could you take a look at the linked bug? There are error log examples and some investigation we did. https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... File chrome/browser/resources/chromeos/login/login_shared.js (right): https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... chrome/browser/resources/chromeos/login/login_shared.js:295: Oobe.skipToLoginForTesting = function() { On 2017/04/07 08:56:02, achuithb wrote: > We have a number of testing methods here. Can you move this to down here? Do you mean moving readyForTesting like PS2? Assuming so, done. If I'm wrong, please fix me.
On 2017/04/07 09:09:41, hidehiko wrote: > Thank you for review. > > > Is there an example of where this is a problem? > > Could you take a look at the linked bug? > There are error log examples and some investigation we did. > > https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... > File chrome/browser/resources/chromeos/login/login_shared.js (right): > > https://codereview.chromium.org/2805033002/diff/20001/chrome/browser/resource... > chrome/browser/resources/chromeos/login/login_shared.js:295: > Oobe.skipToLoginForTesting = function() { > On 2017/04/07 08:56:02, achuithb wrote: > > We have a number of testing methods here. Can you move this to down here? > > Do you mean moving readyForTesting like PS2? Assuming so, done. If I'm wrong, > please fix me. Oops. I meant, PS3.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
On 2017/04/07 18:23:32, achuithb wrote: > lgtm You are also going to need to land a catapult-side change. I assume you will take care of it?
Thank you for review! Submitting. On 2017/04/07 18:24:32, achuithb wrote: > On 2017/04/07 18:23:32, achuithb wrote: > > lgtm > > You are also going to need to land a catapult-side change. I assume you will > take care of it? Yes. this is the CL. https://codereview.chromium.org/2803043002/ PTAL at it, too?
The CQ bit was checked by hidehiko@chromium.org
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": 40001, "attempt_start_ts": 1491809171623040,
"parent_rev": "4e926ec962c82ee6f61f1208b4bf18411d605a59", "commit_rev":
"d5027325193408828b4dab341dabbc6ff0b7791e"}
Message was sent while issue was closed.
Description was changed from ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 BUG=709041 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix flaky NPE in oobe.js. Under some racy condition, telemetry accesses the DOM object before ready. This CL adds the variable representing whether DOM gets loaded. BUG=b/35306355 BUG=709041 TEST=Ran trybots. Apply this and corresponding patch, and ran on ChromeOS device. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2805033002 Cr-Commit-Position: refs/heads/master@{#463191} Committed: https://chromium.googlesource.com/chromium/src/+/d5027325193408828b4dab341dab... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d5027325193408828b4dab341dab... |
