Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2400163002:
Tentative fix for scroll event related crashes on the NTP (Closed)
|
Created:
4 years, 2 months ago by dgn Modified:
4 years, 2 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, ntp-dev+reviews_chromium.org, agrieve+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTentative fix for scroll event related crashes on the NTP
A crash[1] seems to be caused by events attempting to get data from
the NTP while it is not the current page anymore. This CL adds
a check to ensure the page processing the events is the currently
displayed NTP.
[1]: https://crbug.com/649670#c17
BUG=649670
Committed: https://crrev.com/83034cc5fb53f6d66f73f3a8afbfdd0d14b42943
Cr-Commit-Position: refs/heads/master@{#423858}
Patch Set 1 #
Total comments: 6
Patch Set 2 : fix tests #Patch Set 3 : address comments #Patch Set 4 : address comment #
Messages
Total messages: 29 (18 generated)
The CQ bit was checked by dgn@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...
dgn@chromium.org changed reviewers: + bauerb@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:501: // (e.g. delayed recyclerView change notifications) trigger calls to these methods after Nit: RecyclerView capitalized. https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1140: return nativePage != null Is the null check necessary?
The CQ bit was checked by dgn@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 checked by dgn@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...
https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1140: return nativePage != null On 2016/10/07 13:30:19, Bernhard Bauer wrote: > Is the null check necessary? getNewTabPageForCurrentTab() can return null, so I just want to make sure we don't ever run into an issue because of that. It would mean there is no current NTP at all. NativePage should not be null, but who knows. That would be giving too much credit to the input data. Rewrote that bit to hopefully make the intent clearer.
PTAL https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ntp/NewTabPageView.java:501: // (e.g. delayed recyclerView change notifications) trigger calls to these methods after On 2016/10/07 13:30:19, Bernhard Bauer wrote: > Nit: RecyclerView capitalized. Done.
The CQ bit was checked by dgn@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...
https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1140: return nativePage != null On 2016/10/07 13:49:57, dgn wrote: > On 2016/10/07 13:30:19, Bernhard Bauer wrote: > > Is the null check necessary? > > getNewTabPageForCurrentTab() can return null, so I just want to make sure we > don't ever run into an issue because of that. It would mean there is no current > NTP at all. > > NativePage should not be null, but who knows. That would be giving too much > credit to the input data. That's what assert is for :) TBH, I would rather `assert nativePage != null` and leave the `currentNtp != null` check out. If you don't expect anyone to pass in null, there is no need to deal with that case in production. > Rewrote that bit to hopefully make the intent clearer.
Oh, also: can you post the old crashing stack trace somewhere? It can be the bug (or maybe a new bug), but it would be good to have it for posterity.
Description was changed from ========== Tentative fix for scroll event related crashes on the NTP A crash seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. BUG=649670 ========== to ========== Tentative fix for scroll event related crashes on the NTP A crash[1] seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. [1]: https://crbug.com/649670#c17 BUG=649670 ==========
The CQ bit was checked by dgn@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...
PTAL Added link to the comment with the stack trace in the description. https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java (right): https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1140: return nativePage != null On 2016/10/07 14:00:51, Bernhard Bauer wrote: > On 2016/10/07 13:49:57, dgn wrote: > > On 2016/10/07 13:30:19, Bernhard Bauer wrote: > > > Is the null check necessary? > > > > getNewTabPageForCurrentTab() can return null, so I just want to make sure we > > don't ever run into an issue because of that. It would mean there is no > current > > NTP at all. > > > > NativePage should not be null, but who knows. That would be giving too much > > credit to the input data. > > That's what assert is for :) > but they never run >.< All right, I changed to that. > TBH, I would rather `assert nativePage != null` and leave the `currentNtp != > null` check out. If you don't expect anyone to pass in null, there is no need to > deal with that case in production. > > > Rewrote that bit to hopefully make the intent clearer. >
LGTM! On 2016/10/07 14:14:16, dgn wrote: > PTAL > > Added link to the comment with the stack trace in the description. > > https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... > File > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java > (right): > > https://codereview.chromium.org/2400163002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/omnibox/LocationBarLayout.java:1140: > return nativePage != null > On 2016/10/07 14:00:51, Bernhard Bauer wrote: > > On 2016/10/07 13:49:57, dgn wrote: > > > On 2016/10/07 13:30:19, Bernhard Bauer wrote: > > > > Is the null check necessary? > > > > > > getNewTabPageForCurrentTab() can return null, so I just want to make sure we > > > don't ever run into an issue because of that. It would mean there is no > > current > > > NTP at all. > > > > > > NativePage should not be null, but who knows. That would be giving too much > > > credit to the input data. > > > > That's what assert is for :) > > > but they never run >.< 😢 > All right, I changed to that. > > > > TBH, I would rather `assert nativePage != null` and leave the `currentNtp != > > null` check out. If you don't expect anyone to pass in null, there is no need > to > > deal with that case in production. > > > > > Rewrote that bit to hopefully make the intent clearer. > >
The CQ bit was unchecked by dgn@chromium.org
The CQ bit was checked by dgn@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Tentative fix for scroll event related crashes on the NTP A crash[1] seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. [1]: https://crbug.com/649670#c17 BUG=649670 ========== to ========== Tentative fix for scroll event related crashes on the NTP A crash[1] seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. [1]: https://crbug.com/649670#c17 BUG=649670 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Tentative fix for scroll event related crashes on the NTP A crash[1] seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. [1]: https://crbug.com/649670#c17 BUG=649670 ========== to ========== Tentative fix for scroll event related crashes on the NTP A crash[1] seems to be caused by events attempting to get data from the NTP while it is not the current page anymore. This CL adds a check to ensure the page processing the events is the currently displayed NTP. [1]: https://crbug.com/649670#c17 BUG=649670 Committed: https://crrev.com/83034cc5fb53f6d66f73f3a8afbfdd0d14b42943 Cr-Commit-Position: refs/heads/master@{#423858} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/83034cc5fb53f6d66f73f3a8afbfdd0d14b42943 Cr-Commit-Position: refs/heads/master@{#423858} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
