Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2264233002:
Fix a crash in DownloadUIAddapter that happens when Java bridge is created/removed quickly while (Closed)
|
Created:
4 years, 4 months ago by Dmitry Titov Modified:
4 years, 4 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix a crash in DownloadUIAddapter that happens when Java bridge is created/removed quickly while
call to async GetAllPages is still in flight.
BUG=639931
Committed: https://crrev.com/c1747c84cb590b1d7361cc8b70ff5ac3ec34ce42
Cr-Commit-Position: refs/heads/master@{#413654}
Patch Set 1 #
Total comments: 2
Patch Set 2 : addtri-value state #Patch Set 3 : move AddObserver #
Total comments: 6
Patch Set 4 : fix the test #
Total comments: 1
Patch Set 5 : more checks #
Messages
Total messages: 30 (16 generated)
The CQ bit was checked by dimich@chromium.org to run a CQ dry run
dimich@chromium.org changed reviewers: + fgorski@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dimich@chromium.org changed reviewers: + dewittj@chromium.org - fgorski@chromium.org
Change reviewer to dewittj@
basic fix lgtm, some more questions about other dynamics of this class https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:142: model_->AddObserver(this); I think you'll want to AddObserver after the GetAllPages returns. If you imagine a prior operation starting to add a page right before this happens, you might get an observer method firing before OnOfflinePagesLoaded, which would probably cause a DCHECK in OnOfflinePagesLoaded https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... components/offline_pages/downloads/download_ui_adapter.cc:143: model_->GetAllPages( do you want a loading_ indicator so you don't GetAllPages multiple times?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, good catches both. On 2016/08/22 21:54:07, dewittj wrote: > basic fix lgtm, some more questions about other dynamics of this class > > https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... > File components/offline_pages/downloads/download_ui_adapter.cc (right): > > https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... > components/offline_pages/downloads/download_ui_adapter.cc:142: > model_->AddObserver(this); > I think you'll want to AddObserver after the GetAllPages returns. If you > imagine a prior operation starting to add a page right before this happens, you > might get an observer method firing before OnOfflinePagesLoaded, which would > probably cause a DCHECK in OnOfflinePagesLoaded Done > > https://codereview.chromium.org/2264233002/diff/1/components/offline_pages/do... > components/offline_pages/downloads/download_ui_adapter.cc:143: > model_->GetAllPages( > do you want a loading_ indicator so you don't GetAllPages multiple times? Done
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2264233002/#ps40001 (title: "move AddObserver")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
overall looks good, but a few comments to consider. https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:55: ++observers_count_; It may make sense to use HasObserver as a guard when adding and removing the observers, so that your observer count does not get out of sync with reality. E.g. some code adds/removes itself twice. https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:151: state_ = State::NOT_LOADED; perhaps invalidate weak pointers here to prevent any callbacks? (Which would be a more canonical way of handling the all observers unsubscribed scenario... But I think your current approach works too. https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:296: nit: remove extra space.
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org Link to the patchset: https://codereview.chromium.org/2264233002/#ps60001 (title: "fix the test")
The CQ bit was unchecked by dimich@chromium.org
https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:55: ++observers_count_; On 2016/08/22 22:52:22, fgorski wrote: > It may make sense to use HasObserver as a guard when adding and removing the > observers, so that your observer count does not get out of sync with reality. > > E.g. some code adds/removes itself twice. Done. Indeed. Crazy. https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:151: state_ = State::NOT_LOADED; On 2016/08/22 22:52:22, fgorski wrote: > perhaps invalidate weak pointers here to prevent any callbacks? (Which would be > a more canonical way of handling the all observers unsubscribed scenario... > But I think your current approach works too. Ack. But I think that also will stop other operations, like DeleteItem, which would be unintended. https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter_unittest.cc (right): https://codereview.chromium.org/2264233002/diff/40001/components/offline_page... components/offline_pages/downloads/download_ui_adapter_unittest.cc:296: On 2016/08/22 22:52:22, fgorski wrote: > nit: remove extra space. Done.
lgtm with comment. https://codereview.chromium.org/2264233002/diff/60001/components/offline_page... File components/offline_pages/downloads/download_ui_adapter.cc (right): https://codereview.chromium.org/2264233002/diff/60001/components/offline_page... components/offline_pages/downloads/download_ui_adapter.cc:68: void DownloadUIAdapter::RemoveObserver(Observer* observer) { what about here? if (observers_.HasObserver(observer))
On 2016/08/23 01:31:58, fgorski wrote: > lgtm with comment. > > https://codereview.chromium.org/2264233002/diff/60001/components/offline_page... > File components/offline_pages/downloads/download_ui_adapter.cc (right): > > https://codereview.chromium.org/2264233002/diff/60001/components/offline_page... > components/offline_pages/downloads/download_ui_adapter.cc:68: void > DownloadUIAdapter::RemoveObserver(Observer* observer) { > what about here? > if (observers_.HasObserver(observer)) It makes code look pretty paranoid. But I see the rational here! Added :(
The CQ bit was checked by dimich@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org, fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2264233002/#ps80001 (title: "more checks")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix a crash in DownloadUIAddapter that happens when Java bridge is created/removed quickly while call to async GetAllPages is still in flight. BUG=639931 ========== to ========== Fix a crash in DownloadUIAddapter that happens when Java bridge is created/removed quickly while call to async GetAllPages is still in flight. BUG=639931 Committed: https://crrev.com/c1747c84cb590b1d7361cc8b70ff5ac3ec34ce42 Cr-Commit-Position: refs/heads/master@{#413654} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/c1747c84cb590b1d7361cc8b70ff5ac3ec34ce42 Cr-Commit-Position: refs/heads/master@{#413654} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
