Implement WebContentsObserver::WebContentsDestroyed in TabLifecycleUnit |
||||
Issue descriptionThere's some situation that can led to a WebContents being destroyed without a call to TabStripModelObserver::TabClosingAt, and it seems like the bookkeeping done in TabLifecycleUnitSource is entirely based on the TabStripModelObserver::Tab* functions, so we might end up in some situations where TabLifecycleUnitSource::tabs_ contains an entry for a destroyed WebContents. TabLifecycleUnit inherit from content::WebContentsObserver and should maybe implement WebContentsObserver::WebContentsDestroyed to be notified when its WebContents get destroyed, then this information should be forwarded to the TabLifecycleUnitSource that contains this TabLifecycleUnit so it can be removed. This is not super clear how to do this with the current design, as WebContentsObserver::WebContentsDestroyed gets called for all the WebContents (including the ones for which we got a TabStripModelObserver::TabClosingAt notification). One option would be to move the WebContentsObserver out of TabLifecycleUnit (e.g. to a new TabLifecycleUnitWebContentsObserver class), and make TabLifecycleUnitSource own one instance of this for each WebContents it tracks, this is the approach that we took in TabStatsTracker, see how web_contents_usage_observers_ gets used as an example: https://cs.chromium.org/chromium/src/chrome/browser/metrics/tab_stats_tracker.cc?sq=package:chromium&l=265 (WebContents stop being tracked only when we get the WebContentsDestroyed signal)
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b commit 70546dc6e0acea7bc98b3ce948cf1e72c0c5768b Author: Sebastien Marchand <sebmarchand@chromium.org> Date: Wed Mar 07 20:05:32 2018 Fix some tab lifetime management issues in TabLifecycleUnitSource. Move the WebContentsObserver from TabLifeCycleUnit to TabLifeCycleUnitSource, this allows for a better tracking of the WebContents lifetime, in some situation a WebContents might get detached from the TabStrip and then destroyed, which mean that we won't get a TabClosingAt notification for this tab destruction. Another solution would be to implement the TabStripModelObserver::TabDetachedAt function and track the tabs which are in a detached state but this is slightly more complex because TabDetachedAt might be called for several reasons: - A TabDetachedAt usually come after a TabClosedAt event. - TabDetachedAt might be followed by TabInsertedAt, or not if it get destroyed. because of this we won't know if we should keep the TabLifeCycleUnit for this WebContents around (i.e. if it'll get re-inserted in a tab strip) or destroy it because it's being destroyed. Observing WebContentsObserver::WebContentsDestroyed and moving the logic that was in TabClosingAt to this method address these issues, it's the same approach than the one we took in TabStatsTracker. Bug: 819352 , 818454 Change-Id: Ibd3fe49b2798ade19ee781cb70eb30e52372d686 Reviewed-on: https://chromium-review.googlesource.com/952405 Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> Reviewed-by: Chris Hamilton <chrisha@chromium.org> Cr-Commit-Position: refs/heads/master@{#541540} [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_lifecycle_unit.h [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc [modify] https://crrev.com/70546dc6e0acea7bc98b3ce948cf1e72c0c5768b/chrome/browser/resource_coordinator/tab_manager.cc
,
Mar 7 2018
,
Mar 7 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ce49bf6c9fa15caa6865675650747aaf594002cc commit ce49bf6c9fa15caa6865675650747aaf594002cc Author: Sebastien Marchand <sebmarchand@chromium.org> Date: Wed Mar 07 21:24:41 2018 Fix some tab lifetime management issues in TabLifecycleUnitSource. Move the WebContentsObserver from TabLifeCycleUnit to TabLifeCycleUnitSource, this allows for a better tracking of the WebContents lifetime, in some situation a WebContents might get detached from the TabStrip and then destroyed, which mean that we won't get a TabClosingAt notification for this tab destruction. Another solution would be to implement the TabStripModelObserver::TabDetachedAt function and track the tabs which are in a detached state but this is slightly more complex because TabDetachedAt might be called for several reasons: - A TabDetachedAt usually come after a TabClosedAt event. - TabDetachedAt might be followed by TabInsertedAt, or not if it get destroyed. because of this we won't know if we should keep the TabLifeCycleUnit for this WebContents around (i.e. if it'll get re-inserted in a tab strip) or destroy it because it's being destroyed. Observing WebContentsObserver::WebContentsDestroyed and moving the logic that was in TabClosingAt to this method address these issues, it's the same approach than the one we took in TabStatsTracker. Bug: 819352 , 818454 Change-Id: Ibd3fe49b2798ade19ee781cb70eb30e52372d686 Reviewed-on: https://chromium-review.googlesource.com/952405 Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> Reviewed-by: Chris Hamilton <chrisha@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#541540}(cherry picked from commit 70546dc6e0acea7bc98b3ce948cf1e72c0c5768b) Reviewed-on: https://chromium-review.googlesource.com/953367 Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#74} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_lifecycle_unit.h [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc [modify] https://crrev.com/ce49bf6c9fa15caa6865675650747aaf594002cc/chrome/browser/resource_coordinator/tab_manager.cc
,
Mar 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f54ca247bfedb186eb723f65dd5c78503f2487a commit 6f54ca247bfedb186eb723f65dd5c78503f2487a Author: kylechar <kylechar@chromium.org> Date: Thu Mar 08 15:37:37 2018 Revert "Fix some tab lifetime management issues in TabLifecycleUnitSource." This reverts commit 70546dc6e0acea7bc98b3ce948cf1e72c0c5768b. Reason for revert: Causing flaky crashes on waterfall, see https://crbug.com/820075 . Original change's description: > Fix some tab lifetime management issues in TabLifecycleUnitSource. > > Move the WebContentsObserver from TabLifeCycleUnit to TabLifeCycleUnitSource, > this allows for a better tracking of the WebContents lifetime, in some > situation a WebContents might get detached from the TabStrip and then > destroyed, which mean that we won't get a TabClosingAt notification for > this tab destruction. > > Another solution would be to implement the TabStripModelObserver::TabDetachedAt > function and track the tabs which are in a detached state but this is slightly > more complex because TabDetachedAt might be called for several reasons: > - A TabDetachedAt usually come after a TabClosedAt event. > - TabDetachedAt might be followed by TabInsertedAt, or not if it get destroyed. > because of this we won't know if we should keep the TabLifeCycleUnit for this > WebContents around (i.e. if it'll get re-inserted in a tab strip) or destroy > it because it's being destroyed. > > Observing WebContentsObserver::WebContentsDestroyed and moving the logic that > was in TabClosingAt to this method address these issues, it's the same approach > than the one we took in TabStatsTracker. > > Bug: 819352 , 818454 > Change-Id: Ibd3fe49b2798ade19ee781cb70eb30e52372d686 > Reviewed-on: https://chromium-review.googlesource.com/952405 > Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> > Reviewed-by: Chris Hamilton <chrisha@chromium.org> > Cr-Commit-Position: refs/heads/master@{#541540} TBR=chrisha@chromium.org,sebmarchand@chromium.org Change-Id: I0aa17b4db9f4c1468e190ef4ec0dc86aeb08c3a8 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 819352 , 818454 Reviewed-on: https://chromium-review.googlesource.com/955882 Reviewed-by: kylechar <kylechar@chromium.org> Commit-Queue: kylechar <kylechar@chromium.org> Cr-Commit-Position: refs/heads/master@{#541802} [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_lifecycle_unit.h [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc [modify] https://crrev.com/6f54ca247bfedb186eb723f65dd5c78503f2487a/chrome/browser/resource_coordinator/tab_manager.cc
,
Mar 8 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9048ce5d75ebf45fecdd89c4d604e9010a141f7 commit b9048ce5d75ebf45fecdd89c4d604e9010a141f7 Author: kylechar <kylechar@chromium.org> Date: Thu Mar 08 16:13:45 2018 Revert "Fix some tab lifetime management issues in TabLifecycleUnitSource." This reverts commit 70546dc6e0acea7bc98b3ce948cf1e72c0c5768b. Reason for revert: Causing flaky crashes on waterfall, see https://crbug.com/820075 . Original change's description: > Fix some tab lifetime management issues in TabLifecycleUnitSource. > > Move the WebContentsObserver from TabLifeCycleUnit to TabLifeCycleUnitSource, > this allows for a better tracking of the WebContents lifetime, in some > situation a WebContents might get detached from the TabStrip and then > destroyed, which mean that we won't get a TabClosingAt notification for > this tab destruction. > > Another solution would be to implement the TabStripModelObserver::TabDetachedAt > function and track the tabs which are in a detached state but this is slightly > more complex because TabDetachedAt might be called for several reasons: > - A TabDetachedAt usually come after a TabClosedAt event. > - TabDetachedAt might be followed by TabInsertedAt, or not if it get destroyed. > because of this we won't know if we should keep the TabLifeCycleUnit for this > WebContents around (i.e. if it'll get re-inserted in a tab strip) or destroy > it because it's being destroyed. > > Observing WebContentsObserver::WebContentsDestroyed and moving the logic that > was in TabClosingAt to this method address these issues, it's the same approach > than the one we took in TabStatsTracker. > > Bug: 819352 , 818454 > Change-Id: Ibd3fe49b2798ade19ee781cb70eb30e52372d686 > Reviewed-on: https://chromium-review.googlesource.com/952405 > Commit-Queue: Sébastien Marchand <sebmarchand@chromium.org> > Reviewed-by: Chris Hamilton <chrisha@chromium.org> > Cr-Commit-Position: refs/heads/master@{#541540} TBR=chrisha@chromium.org,sebmarchand@chromium.org Change-Id: I0aa17b4db9f4c1468e190ef4ec0dc86aeb08c3a8 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 819352 , 818454 Reviewed-on: https://chromium-review.googlesource.com/955882 Reviewed-by: kylechar <kylechar@chromium.org> Commit-Queue: kylechar <kylechar@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#541802}(cherry picked from commit 6f54ca247bfedb186eb723f65dd5c78503f2487a) Reviewed-on: https://chromium-review.googlesource.com/955942 Reviewed-by: Sébastien Marchand <sebmarchand@chromium.org> Cr-Commit-Position: refs/branch-heads/3359@{#99} Cr-Branched-From: 66afc5e5d10127546cc4b98b9117aff588b5e66b-refs/heads/master@{#540276} [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_lifecycle_unit.cc [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_lifecycle_unit.h [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.cc [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_lifecycle_unit_source.h [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_lifecycle_unit_source_unittest.cc [modify] https://crrev.com/b9048ce5d75ebf45fecdd89c4d604e9010a141f7/chrome/browser/resource_coordinator/tab_manager.cc
,
Mar 8 2018
|
||||
►
Sign in to add a comment |
||||
Comment 1 by sebmarchand@chromium.org
, Mar 7 2018Owner: sebmarchand@chromium.org
Status: Assigned (was: Untriaged)