Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
179123002:
Fix bug causing blank pages on resume of Chrome for Android. (Closed)
|
Created:
6 years, 9 months ago by Yaron Modified:
6 years, 6 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, ppi, Ted C, klobag.chromium, joth Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionFix bug causing blank pages on resume of Chrome for Android.
https://codereview.chromium.org/14297022 had a faulty assumption that
RenderProcessHost would be unique in the face of the underly
RenderProcess crashing. This is not the case, so listen for the
appropriate callback on RenderProcessHostObserver until the refactoring
discussed in the above is complete.
BUG=321610
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256153
Patch Set 1 #Patch Set 2 : reitveld #
Total comments: 10
Patch Set 3 : #
Total comments: 4
Patch Set 4 : Make SuspendedProcessWatcher a full-fledge class and clean up properly #Patch Set 5 : add dchecks for strictness #
Total comments: 2
Patch Set 6 : add hasconnection check #
Total comments: 1
Messages
Total messages: 22 (0 generated)
This is a continuation of https://codereview.chromium.org/14297022 in spirit. While it doesn't do the clean-up (that seems advisable), it addresses a case missed in the original fix. Namely that a crashed renderer process (or one evicted by the android system), will use the same RPH. This can result in an over-resumed renderer (as discussed in the previous CL) and pages like Facebook and Mobile Gmail not rendering anything on resume as they're drawn by JS.
Thanks Yaron, great find! LGTM https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:38: base::LazyInstance<std::vector<int /* process id */> > g_suspended_processes = Nit: s/process id/RPH ID (see my comment below). https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:41: // Listens for RenderProcessHosts who's renderers have crashed. This is You mean in your CL description that the same RenderProcessHost instance is kept alive when a renderer process gets killed and re-spawned, right? It looks to me that we may have had this in mind when landing this CL. We did completely screw up though by using RenderProcessHost::GetID(). By looking at RenderProcessHostImpl the ID is constant (and set to the initial renderer process PID) as opposed to the current renderer process' PID backing the RPH (as I/we were expecting). Having a non-constant ID would not be sane either though :) I suspect that we should have used RPH::GetHandle() in the first place as opposed to RPH::GetID() but there is no RPH::FromHandle() method... So removing the RPH IDs from the vector as the renderer processes get killed like you did looks like the best/only solution IMO (modulo doing the refactoring mentioned line 26 of course :). https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:41: // Listens for RenderProcessHosts who's renderers have crashed. This is Nit: s/who's/whose I believe although my knowledge in English is quite limited :) https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:46: base::ProcessHandle handle, Nit: extra leading space here and on the two lines below. https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:60: base::LazyInstance<SuspendedProcessWatcher > g_suspended_processes_watcher = Nit: extra space before '>'.
https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:38: base::LazyInstance<std::vector<int /* process id */> > g_suspended_processes = On 2014/02/25 09:41:12, Philippe wrote: > Nit: s/process id/RPH ID (see my comment below). Done. https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:41: // Listens for RenderProcessHosts who's renderers have crashed. This is On 2014/02/25 09:41:12, Philippe wrote: > Nit: s/who's/whose I believe although my knowledge in English is quite limited > :) Done. https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:41: // Listens for RenderProcessHosts who's renderers have crashed. This is On 2014/02/25 09:41:12, Philippe wrote: > You mean in your CL description that the same RenderProcessHost instance is kept > alive when a renderer process gets killed and re-spawned, right? > > It looks to me that we may have had this in mind when landing this CL. We did > completely screw up though by using RenderProcessHost::GetID(). By looking at > RenderProcessHostImpl the ID is constant (and set to the initial renderer > process PID) as opposed to the current renderer process' PID backing the RPH (as > I/we were expecting). Having a non-constant ID would not be sane either though > :) I suspect that we should have used RPH::GetHandle() in the first place as > opposed to RPH::GetID() but there is no RPH::FromHandle() method... > > So removing the RPH IDs from the vector as the renderer processes get killed > like you did looks like the best/only solution IMO (modulo doing the refactoring > mentioned line 26 of course :). It's not actually the renderer process pid. It's just a monotonically increasing id. Ya, I came to the same conclusion about GetHandle when looking for an alternate way. https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:46: base::ProcessHandle handle, On 2014/02/25 09:41:12, Philippe wrote: > Nit: extra leading space here and on the two lines below. Done. https://codereview.chromium.org/179123002/diff/20001/content/browser/android/... content/browser/android/content_view_statics.cc:60: base::LazyInstance<SuspendedProcessWatcher > g_suspended_processes_watcher = On 2014/02/25 09:41:12, Philippe wrote: > Nit: extra space before '>'. Done.
The CQ bit was checked by yfriedman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/179123002/40001
late lgtm, thanks!
On 2014/02/25 18:21:24, bulach wrote: > late lgtm, thanks! I don't think that this can cause a problem in the WebView as we're single process. Is that right? Just trying to see if merged are required.
On 2014/02/25 18:31:37, benm wrote: > On 2014/02/25 18:21:24, bulach wrote: > > late lgtm, thanks! > > I don't think that this can cause a problem in the WebView as we're single > process. Is that right? Just trying to see if merged are required. That's right. This only improves the case when a renderer crashes/gets evicted and then you reload the page.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/179123002/40001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/179123002/40001
https://chromiumcodereview.appspot.com/179123002/diff/40001/content/browser/a... File content/browser/android/content_view_statics.cc (right): https://chromiumcodereview.appspot.com/179123002/diff/40001/content/browser/a... content/browser/android/content_view_statics.cc:71: host->AddObserver(g_suspended_processes_watcher.Pointer()); Hmm, are we calling this every time in suspend? Are we hitting the following NOTREACHED() if you hit HOME/Chrome/HOME? // Add an observer to the list. An observer should not be added to // the same list more than once. void AddObserver(ObserverType* obs) { if (std::find(observers_.begin(), observers_.end(), obs) != observers_.end()) { NOTREACHED() << "Observers can only be added once!"; return; } observers_.push_back(obs); } https://chromiumcodereview.appspot.com/179123002/diff/40001/content/browser/a... content/browser/android/content_view_statics.cc:80: if (host) // The process might have been killed since it was suspended. Should we assert host not null here?
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/179123002/40001
The CQ bit was unchecked by yfriedman@chromium.org
Thanks, Yaron! How about adding: DCHECK(shared_timer_suspended_ > 0); at the beginning of WebKitPlatformSupportImpl::ResumeSharedTimer() ?
ppi: thats' being handled in https://codereview.chromium.org/181823006/ https://codereview.chromium.org/179123002/diff/40001/content/browser/android/... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/40001/content/browser/android/... content/browser/android/content_view_statics.cc:71: host->AddObserver(g_suspended_processes_watcher.Pointer()); On 2014/02/25 22:19:27, klobag.chromium wrote: > Hmm, are we calling this every time in suspend? Are we hitting the following > NOTREACHED() if you hit HOME/Chrome/HOME? > > // Add an observer to the list. An observer should not be added to > // the same list more than once. > void AddObserver(ObserverType* obs) { > if (std::find(observers_.begin(), observers_.end(), obs) > != observers_.end()) { > NOTREACHED() << "Observers can only be added once!"; > return; > } > observers_.push_back(obs); > } Done. https://codereview.chromium.org/179123002/diff/40001/content/browser/android/... content/browser/android/content_view_statics.cc:80: if (host) // The process might have been killed since it was suspended. On 2014/02/25 22:19:27, klobag.chromium wrote: > Should we assert host not null here? Done.
https://codereview.chromium.org/179123002/diff/80001/content/browser/android/... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/80001/content/browser/android/... content/browser/android/content_view_statics.cc:63: host->AddObserver(this); should we only do so if host has connection? Otherwise, we can still get into the bad state, right?
https://codereview.chromium.org/179123002/diff/80001/content/browser/android/... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/80001/content/browser/android/... content/browser/android/content_view_statics.cc:63: host->AddObserver(this); On 2014/03/11 00:36:19, klobag.chromium wrote: > should we only do so if host has connection? > > Otherwise, we can still get into the bad state, right? Done.
lgtm
The CQ bit was checked by yfriedman@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/yfriedman@chromium.org/179123002/100001
Message was sent while issue was closed.
Change committed as 256153
Message was sent while issue was closed.
Sorry to revive an old CL, but seems to be breaking some WebView compatibility cases ... :-/ https://codereview.chromium.org/179123002/diff/100001/content/browser/android... File content/browser/android/content_view_statics.cc (right): https://codereview.chromium.org/179123002/diff/100001/content/browser/android... content/browser/android/content_view_statics.cc:63: if (host->HasConnection()) { Why do we add this check here? This seems to be causing a compatibility break for WebView where an app calls pauseTimers before navigating to the initial document and expects that page to be loaded with the shared timer paused. Do you think that removing it would cause an issue for Chrome? |
