Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
550973002:
Supervised user interstitial: Don't destroy WebContents that might still be in use (Closed)
|
Created:
6 years, 3 months ago by Marc Treib Modified:
6 years, 3 months ago Reviewers:
Bernhard Bauer CC:
chromium-reviews, pam+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionSupervised user interstitial: Don't destroy WebContents that might still be in use.
Before, the WebContents was closed directly from OnDontProceed. This breaks if the caller expects the WebContents to survive (happened e.g. when a new interstitial was being initialized).
This CL instead posts a task to the message loop.
Also, the tab won't be closed anymore if it's the last one in the window, as that would lead to closing the window.
BUG=411841
Committed: https://crrev.com/e7ccb7eb32d74ae96f8f6ee7b038a02c4cffe491
Cr-Commit-Position: refs/heads/master@{#293985}
Patch Set 1 #Patch Set 2 : fix #
Total comments: 11
Patch Set 3 : WebContentsUserData #
Total comments: 8
Patch Set 4 : only close on "go back" #
Total comments: 2
Patch Set 5 : review comments #Patch Set 6 : call directly on "back" #Patch Set 7 : fix test #Patch Set 8 : fix Android build #
Total comments: 3
Patch Set 9 : review comment #
Messages
Total messages: 30 (6 generated)
treib@chromium.org changed reviewers: + bauerb@chromium.org
That was way harder than it should have been... PTAL!
Please use a more descriptive CL titld than "Fix a crash". What are you actually changing to fix the crash? https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; If you want to tie the lifetime of this object to the lifetime of the WebContents, you can use WebContentsUserData<>. https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:84: DCHECK_NE(index, TabStripModel::kNoTab); Switch arguments please. https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:308: TabCloser::CloseTab(web_contents_); This is a bit weird. The WebContents might get closed on its own between now and the next time the message loop runs, but we might also need it to stay around? Could you get me that stack trace maybe, so I can take a closer look at what happens here? If it's just about canceling the blocked requests, we could add an additional flag here to distinguish between an explicit user action to go back, and being canceled for another reason.
https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/08 16:29:46, Bernhard Bauer wrote: > If you want to tie the lifetime of this object to the lifetime of the > WebContents, you can use WebContentsUserData<>. That does seem less cumbersome, thanks! Looking... https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:84: DCHECK_NE(index, TabStripModel::kNoTab); On 2014/09/08 16:29:46, Bernhard Bauer wrote: > Switch arguments please. Done. https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:308: TabCloser::CloseTab(web_contents_); On 2014/09/08 16:29:46, Bernhard Bauer wrote: > This is a bit weird. The WebContents might get closed on its own between now and > the next time the message loop runs, but we might also need it to stay around? > Could you get me that stack trace maybe, so I can take a closer look at what > happens here? If it's just about canceling the blocked requests, we could add an > additional flag here to distinguish between an explicit user action to go back, > and being canceled for another reason. "we" = this interstitial? Then I don't see why we might still need the WebContents. We're in the process of being closed. I've posted a stack trace + explanation on the bug. Not sure what you mean about canceling blocked requests? All that's already done by this point.
https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/09 09:17:09, Marc Treib wrote: > On 2014/09/08 16:29:46, Bernhard Bauer wrote: > > If you want to tie the lifetime of this object to the lifetime of the > > WebContents, you can use WebContentsUserData<>. > > That does seem less cumbersome, thanks! Looking... Hm, on closer inspection: Only a single instance of a given class can be attached as UserData. We "should" not actually ever get more than one, but it does seem fragile. I'd prefer to stay with the observer.
https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/09 09:30:47, Marc Treib wrote: > On 2014/09/09 09:17:09, Marc Treib wrote: > > On 2014/09/08 16:29:46, Bernhard Bauer wrote: > > > If you want to tie the lifetime of this object to the lifetime of the > > > WebContents, you can use WebContentsUserData<>. > > > > That does seem less cumbersome, thanks! Looking... > > Hm, on closer inspection: Only a single instance of a given class can be > attached as UserData. We "should" not actually ever get more than one, but it > does seem fragile. I'd prefer to stay with the observer. Um... if we were to attach two instances of this observer to a WebContents, we would close it twice, wouldn't we?
https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/09 09:41:55, Bernhard Bauer wrote: > On 2014/09/09 09:30:47, Marc Treib wrote: > > On 2014/09/09 09:17:09, Marc Treib wrote: > > > On 2014/09/08 16:29:46, Bernhard Bauer wrote: > > > > If you want to tie the lifetime of this object to the lifetime of the > > > > WebContents, you can use WebContentsUserData<>. > > > > > > That does seem less cumbersome, thanks! Looking... > > > > Hm, on closer inspection: Only a single instance of a given class can be > > attached as UserData. We "should" not actually ever get more than one, but it > > does seem fragile. I'd prefer to stay with the observer. > > Um... if we were to attach two instances of this observer to a WebContents, we > would close it twice, wouldn't we? No, the second one would get WebContentsDestroyed and delete itself first.
Whoops, forgot to publish comments :) https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/09 09:44:05, Marc Treib wrote: > On 2014/09/09 09:41:55, Bernhard Bauer wrote: > > On 2014/09/09 09:30:47, Marc Treib wrote: > > > On 2014/09/09 09:17:09, Marc Treib wrote: > > > > On 2014/09/08 16:29:46, Bernhard Bauer wrote: > > > > > If you want to tie the lifetime of this object to the lifetime of the > > > > > WebContents, you can use WebContentsUserData<>. > > > > > > > > That does seem less cumbersome, thanks! Looking... > > > > > > Hm, on closer inspection: Only a single instance of a given class can be > > > attached as UserData. We "should" not actually ever get more than one, but > it > > > does seem fragile. I'd prefer to stay with the observer. > > > > Um... if we were to attach two instances of this observer to a WebContents, we > > would close it twice, wouldn't we? > > No, the second one would get WebContentsDestroyed and delete itself first. That does not seem less fragile than the WebContentsUserData approach to me. You said yourself that we shouldn't ever get more than one of these, but if that should ever happen, we will silently ignore it. With WebContentsUserData, we can actually DCHECK that there isn't one already attached to it.
WebContentsUserData done. The "only close the tab if the 'back' button was pressed" part still remains. https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/20001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:72: delete this; On 2014/09/09 10:50:27, Bernhard Bauer wrote: > On 2014/09/09 09:44:05, Marc Treib wrote: > > On 2014/09/09 09:41:55, Bernhard Bauer wrote: > > > On 2014/09/09 09:30:47, Marc Treib wrote: > > > > On 2014/09/09 09:17:09, Marc Treib wrote: > > > > > On 2014/09/08 16:29:46, Bernhard Bauer wrote: > > > > > > If you want to tie the lifetime of this object to the lifetime of the > > > > > > WebContents, you can use WebContentsUserData<>. > > > > > > > > > > That does seem less cumbersome, thanks! Looking... > > > > > > > > Hm, on closer inspection: Only a single instance of a given class can be > > > > attached as UserData. We "should" not actually ever get more than one, but > > it > > > > does seem fragile. I'd prefer to stay with the observer. > > > > > > Um... if we were to attach two instances of this observer to a WebContents, > we > > > would close it twice, wouldn't we? > > > > No, the second one would get WebContentsDestroyed and delete itself first. > > That does not seem less fragile than the WebContentsUserData approach to me. You > said yourself that we shouldn't ever get more than one of these, but if that > should ever happen, we will silently ignore it. With WebContentsUserData, we can > actually DCHECK that there isn't one already attached to it. You're right, silently ignoring a second request to close the tab should be fine. Done.
https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:60: friend class content::WebContentsUserData<TabCloser>; Add a blank line afterwards? https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:74: // Don't close the last tab in the window. Nit: Can you move the comment into the else branch? Otherwise from a casual glance it looks like this refers to the if-branch. https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:91: DEFINE_WEB_CONTENTS_USER_DATA_KEY(TabCloser); Can we put this into the anonymous namespace? https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:303: TabCloser::CloseTab(web_contents_); Directly call TabCloser::CreateForContents()? Alternatively, keep the method, but then why not check that there really isn't already one attached? That way you can be more confident about that assumption.
..aand here's the "only close when the user pressed back" part :)
https://codereview.chromium.org/550973002/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:304: if (back_button_pressed_ && !continue_request && nav_entry_count == 0) Just call this directly from CommandReceived(). If you feel that's too ugly, wrap it in a method OnBackSelected or something.
https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:60: friend class content::WebContentsUserData<TabCloser>; On 2014/09/09 11:26:06, Bernhard Bauer wrote: > Add a blank line afterwards? Done. https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:74: // Don't close the last tab in the window. On 2014/09/09 11:26:06, Bernhard Bauer wrote: > Nit: Can you move the comment into the else branch? Otherwise from a casual > glance it looks like this refers to the if-branch. Done. https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:91: DEFINE_WEB_CONTENTS_USER_DATA_KEY(TabCloser); On 2014/09/09 11:26:06, Bernhard Bauer wrote: > Can we put this into the anonymous namespace? No. In Clang's words: ../../chrome/browser/supervised_user/supervised_user_interstitial.cc:90:1: error: cannot define or redeclare 'kLocatorKey' here because namespace '' does not enclose namespace 'WebContentsUserData<(anonymous namespace)::TabCloser>' (kinda makes sense, since it must be accessible from outside this file/namespace) https://codereview.chromium.org/550973002/diff/40001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:303: TabCloser::CloseTab(web_contents_); On 2014/09/09 11:26:06, Bernhard Bauer wrote: > Directly call TabCloser::CreateForContents()? > > Alternatively, keep the method, but then why not check that there really isn't > already one attached? That way you can be more confident about that assumption. As you said, it doesn't really matter if there already is one; the new one will just be ignored, which is fine. So I've removed the method and now call CreateForWebContents directly. https://codereview.chromium.org/550973002/diff/60001/chrome/browser/supervise... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/60001/chrome/browser/supervise... chrome/browser/supervised_user/supervised_user_interstitial.cc:304: if (back_button_pressed_ && !continue_request && nav_entry_count == 0) On 2014/09/09 11:32:31, Bernhard Bauer wrote: > Just call this directly from CommandReceived(). If you feel that's too ugly, > wrap it in a method OnBackSelected or something. Right, that makes more sense now. Small wrinkle: At this point, the transient navigation entry is still around, so I have to check for count==1 instead of count==0.
lgtm
On 2014/09/09 12:10:47, Bernhard Bauer wrote: > lgtm Thanks for the comments!
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/550973002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_dbg_tests_recipe on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_interstitial.cc:82: tab_strip->CloseWebContentsAt( Do we actually need to do it this way, or could we also call web_contents_->Close()?
https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_interstitial.cc:82: tab_strip->CloseWebContentsAt( On 2014/09/09 14:12:20, Bernhard Bauer wrote: > Do we actually need to do it this way, or could we also call > web_contents_->Close()? Turns out that web_contents_->Close() ends up calling the tab strip anyway (via https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...), so it doesn't really matter. But we still need to get the tab strip to check if there's more than one tab, so.. meh. Oh well, it keeps the difference between Android and non-Android a bit smaller, so, done.
https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_interstitial.cc (right): https://codereview.chromium.org/550973002/diff/140001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_interstitial.cc:82: tab_strip->CloseWebContentsAt( On 2014/09/09 14:28:10, Marc Treib wrote: > On 2014/09/09 14:12:20, Bernhard Bauer wrote: > > Do we actually need to do it this way, or could we also call > > web_contents_->Close()? > > Turns out that web_contents_->Close() ends up calling the tab strip anyway (via > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...), > so it doesn't really matter. But we still need to get the tab strip to check if > there's more than one tab, so.. meh. Oh well, it keeps the difference between > Android and non-Android a bit smaller, so, done. Yeah, exactly.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/550973002/160001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded time limit waiting for builds to trigger.
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/treib@chromium.org/550973002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as 53647b63c8fd10a316a568edad18a2f0d33e1688
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/e7ccb7eb32d74ae96f8f6ee7b038a02c4cffe491 Cr-Commit-Position: refs/heads/master@{#293985} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
