re-use WebUIs

re-use WebUIs

this is a partial revert of r133077 and an alternative fix for bug 121741. The last commit was faulty because a same-document navigate would not send new DocumentAvailableInFrame IPCs, hence all WebUISend messages would be ignored after a same document navigation. An example of a same-document navigation is when the user presses back, and the page handles window.onpopstate.

The new fix:
Re-use WebUI objects for navigations where we re-use the render view, and the old and new URLs return the same WebUIController type. This covers both the reload case and the same-document nav case. This way, the re-used WebUI is still initialized and is able to continue to handle messages without the possibility of unexpected messages causing crashes. WebUI re-builds on every back/forward/reload are also wasteful so this is a performance win.

BUG=123705, 121741, 123710
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=134055

[Evan Stade, 2012-04-20 19:05:44]

not sure who the best reviewer is these days. This seems to be code no one owns.

[jam, 2012-04-20 19:18:51]

I'm not familiar with the webui code in WebContents/RenderViewHostManager, so I
can't review that, but this change looks awesome

[Evan Stade, 2012-04-20 23:12:41]

a little more background:

WebUIs will now outlast their page in the reload case. This means the page may
send "onPageLoaded" or similar messages more than once in a WebUIHandler's
lifetime. Some WebUIHandlers don't like this; I've fixed all the ones I could
find (by going to about:about and going through every page). There's one issue
left, which is that after a page is getting torn down for a refresh, and a new
one takes its place, a WebUIHandler may try to call a javascript function that
is not yet ready. This is sort of the inverse problem from bug 123705. However,
this should be harmless; it can't cause a crash and the worst result is a little
bit of console logging.

[Dan Beam, 2012-04-21 02:53:07]

I'm not qualified to review this deep into the code either, but here's some
style nits (and I didn't really find any issues with the code itself)

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
File chrome/browser/ui/webui/gpu_internals_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
chrome/browser/ui/webui/gpu_internals_ui.cc:90: bool crash_list_available_;
doc comment

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
chrome/browser/ui/webui/tracing_ui.cc:119: 
doc comment

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:420:
(factory->GetWebUIType(controller.GetBrowserContext(),
can you split this into multiple lines?

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:615: if (web_ui_.get()
!= pending_web_ui_)
you don't really need .get() operators for scoped_ptr's IIRC, as they have a T*
operator

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:700: 
why \n?

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:735: // not have it's
bindings set appropriately.
its

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:223: const
content::NavigationEntry* curr_entry,
can you change this to cur_entry or change above to curr_entry?

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:285: // The
pending_web_ui_ may be non-NULL even when the pending_render_view_host_
nit: |pending_web_ui_| and |pending_render_view_host_| if you want to

http://codereview.chromium.org/10154004/diff/14018/content/common/view_messag...
File content/common/view_messages.h (right):

http://codereview.chromium.org/10154004/diff/14018/content/common/view_messag...
content/common/view_messages.h:1396: // Sent when the document element is
available for the toplevel frame.  This
shouldn't this be "top-level"?

[Evan Stade, 2012-04-23 20:09:42]

someone needs to provide a substantive review for this patch. Stuart, can you
take a look since you had to deal with this problem most recently?

[stuartmorgan, 2012-04-24 09:12:20]

It's a bit scary that I'm the most qualified reviewer for this, but okay :)

Conceptually this looks a lot cleaner than my hack (especially now that the
check on the JS side has been changed to be exactly backwards; I'm confused how
that happened), so I'm happy with the structure, but I have concerns about some
implementation details.

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
File chrome/browser/ui/webui/gpu_internals_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
chrome/browser/ui/webui/gpu_internals_ui.cc:104: observing_(true) {
It's initialized to true, and only set to true from what I can see, so it looks
like your new |if| is just |if (!true)|. Am I missing something? Seems like this
should be s/true/false/

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:189:
content::NotificationService::AllSources());
Why this change in ordering? If it's important, it should be commented.

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:217: &NewTabUI::PaintTimeout);
Is doing all this timer stuff a second time okay?

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
chrome/browser/ui/webui/tracing_ui.cc:164: observing_(true) {
Same issue here.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:294: WebUIImpl*
pending_web_ui_;
This memory model makes me *extremely* nervous; I think it's almost certain that
someone will introduce a leak or a crash eventually this way (doubly so since
this class is both very complex and weakly owned).

I can see two sane approaches here:
1) A trivial ref-counted class that wraps WebUIImpl*, so that you can use that
as the type for two scoped_refptrs, or
2) A little helper class that owns both WebUIImpl objects and has a handful of
public methods that do what you need (checks if there is a pending web ui,
returns the pending web ui, commits the pending web ui).

[Evan Stade, 2012-04-24 17:52:40]

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
File chrome/browser/ui/webui/gpu_internals_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
chrome/browser/ui/webui/gpu_internals_ui.cc:90: bool crash_list_available_;
On 2012/04/21 02:53:07, Dan Beam wrote:
> doc comment

Done.

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/gp...
chrome/browser/ui/webui/gpu_internals_ui.cc:104: observing_(true) {
On 2012/04/24 09:12:20, stuartmorgan wrote:
> It's initialized to true, and only set to true from what I can see, so it
looks
> like your new |if| is just |if (!true)|. Am I missing something? Seems like
this
> should be s/true/false/

yea, good catch.

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:189:
content::NotificationService::AllSources());
On 2012/04/24 09:12:20, stuartmorgan wrote:
> Why this change in ordering? If it's important, it should be commented.

change in ordering? it's now in a different function. I can comment why, but the
reason for this (and many other changes) is that we may get OnPageLoaded
multiple times, and registering for the same notification multiple times =>
DCHECK.

(I did it this way instead of adding an |observing_| member because it's rare
that an upgrade recommendation notification would come through, so the page
loading will seldom lose the race against upgrade notification. In the other
cases, such as GpuInternals, the race was happening more often, causing lots of
console spew.)

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/nt...
File chrome/browser/ui/webui/ntp/new_tab_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/nt...
chrome/browser/ui/webui/ntp/new_tab_ui.cc:217: &NewTabUI::PaintTimeout);
On 2012/04/24 09:12:20, stuartmorgan wrote:
> Is doing all this timer stuff a second time okay?

as far as calling start more than once, yea it's ok, it just restarts the timer.

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
File chrome/browser/ui/webui/tracing_ui.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/tr...
chrome/browser/ui/webui/tracing_ui.cc:164: observing_(true) {
On 2012/04/24 09:12:20, stuartmorgan wrote:
> Same issue here.

Done.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.cc (right):

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:420:
(factory->GetWebUIType(controller.GetBrowserContext(),
On 2012/04/21 02:53:07, Dan Beam wrote:
> can you split this into multiple lines?

not easily.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:700: 
On 2012/04/21 02:53:07, Dan Beam wrote:
> why \n?

Done.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.cc:735: // not have it's
bindings set appropriately.
On 2012/04/21 02:53:07, Dan Beam wrote:
> its

Done.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
File content/browser/web_contents/render_view_host_manager.h (right):

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:223: const
content::NavigationEntry* curr_entry,
On 2012/04/21 02:53:07, Dan Beam wrote:
> can you change this to cur_entry or change above to curr_entry?

Done.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:285: // The
pending_web_ui_ may be non-NULL even when the pending_render_view_host_
On 2012/04/21 02:53:07, Dan Beam wrote:
> nit: |pending_web_ui_| and |pending_render_view_host_| if you want to

Done.

http://codereview.chromium.org/10154004/diff/14018/content/browser/web_conten...
content/browser/web_contents/render_view_host_manager.h:294: WebUIImpl*
pending_web_ui_;
On 2012/04/24 09:12:20, stuartmorgan wrote:
> This memory model makes me *extremely* nervous; I think it's almost certain
that
> someone will introduce a leak or a crash eventually this way (doubly so since
> this class is both very complex and weakly owned).
> 
> I can see two sane approaches here:
> 1) A trivial ref-counted class that wraps WebUIImpl*, so that you can use that
> as the type for two scoped_refptrs, or
> 2) A little helper class that owns both WebUIImpl objects and has a handful of
> public methods that do what you need (checks if there is a pending web ui,
> returns the pending web ui, commits the pending web ui).

Done.

http://codereview.chromium.org/10154004/diff/14018/content/common/view_messag...
File content/common/view_messages.h (right):

http://codereview.chromium.org/10154004/diff/14018/content/common/view_messag...
content/common/view_messages.h:1396: // Sent when the document element is
available for the toplevel frame.  This
On 2012/04/21 02:53:07, Dan Beam wrote:
> shouldn't this be "top-level"?

Done.

[stuartmorgan, 2012-04-24 19:20:02]

LGTM

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
File chrome/browser/ui/webui/help/help_handler.cc (right):

http://codereview.chromium.org/10154004/diff/14018/chrome/browser/ui/webui/he...
chrome/browser/ui/webui/help/help_handler.cc:189:
content::NotificationService::AllSources());
On 2012/04/24 17:52:40, Evan Stade wrote:
> change in ordering? it's now in a different function.

Sorry, totally spaced on the elided lines in the review tool, and thought you
just moved it to the start of the same function.

[commit-bot: I haz the power, 2012-04-25 19:25:51]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10154004/17004

[commit-bot: I haz the power, 2012-04-25 19:26:09]

Presubmit check for 10154004-17004 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/browser
    content/common
    content/public/browser
    content/renderer

Presubmit checks took 3.5s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[arv (Not doing code reviews), 2012-04-25 22:10:36]

Would it not be better to recreate the webui when we do reloads? It seems like a
cleaner model to me.

[arv (Not doing code reviews), 2012-04-25 22:12:50]

On Wed, Apr 25, 2012 at 15:10,  <arv@chromium.org> wrote:
> Would it not be better to recreate the webui when we do reloads? It seems
> like a
> cleaner model to me.

Sorry, I should have looked at the code before sending that. I like
the initialized_ solution.

[arv (Not doing code reviews), 2012-04-25 22:25:47]

I'm still concerned. It seems like it would be cleaner if the "life time" of the
client side was in sync with the browser side. We will keep running into
problems if reloads don't work the same as initial load.

[Evan Stade, 2012-04-25 22:29:46]

changed from refptrs to WeakPtrs as I'm informed refptrs are considered harmful.
There have now been 3 memory management models, I happen to prefer the refptr
one, but can live with any of them.

On 2012/04/25 22:12:50, arv wrote:
> On Wed, Apr 25, 2012 at 15:10,  <mailto:arv@chromium.org> wrote:
> > Would it not be better to recreate the webui when we do reloads? It seems
> > like a
> > cleaner model to me.

that leads to crashes, see
http://code.google.com/p/chromium/issues/detail?id=121741

> 
> Sorry, I should have looked at the code before sending that. I like
> the initialized_ solution.

which solution is that?

[Evan Stade, 2012-04-25 22:34:27]

On 2012/04/25 22:25:47, arv wrote:
> I'm still concerned. It seems like it would be cleaner if the "life time" of
the
> client side was in sync with the browser side. We will keep running into
> problems if reloads don't work the same as initial load.

I agree that would be ideal but don't see a way to achieve it.

[arv (Not doing code reviews), 2012-04-25 22:35:42]

On 2012/04/25 22:29:46, Evan Stade wrote:
> > Sorry, I should have looked at the code before sending that. I like
> > the initialized_ solution.
> 
> which solution is that?

Sorry, that was referring to how you do it in this patch.

Maybe I'm missing something but if the sequence of a reload is different from
initial load (on the client side) we are bound to have bugs.

[Evan Stade, 2012-04-25 22:47:03]

On 2012/04/25 22:35:42, arv wrote:
> On 2012/04/25 22:29:46, Evan Stade wrote:
> > > Sorry, I should have looked at the code before sending that. I like
> > > the initialized_ solution.
> > 
> > which solution is that?
> 
> Sorry, that was referring to how you do it in this patch.
> 
> Maybe I'm missing something but if the sequence of a reload is different from
> initial load (on the client side) we are bound to have bugs.

initial load and reload will be the same on client side, the difference is in
the browser only. It's possible there will be bugs in the browser (message
handlers) but they will hopefully be rarer and less likely to be crashes ---
getting "pageLoad" twice should be less harmful than getting messages out of
order, or unexpectedly before "pageLoad". I manually validated on all chrome://
pages I could find via about:about that there were no crashes or DCHECKs on
reload (except those DCHECKs I fixed).

[arv (Not doing code reviews), 2012-04-25 22:55:01]

LGTM

[jam, 2012-04-26 01:09:28]

lgtm

[commit-bot: I haz the power, 2012-04-26 01:25:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/estade@chromium.org/10154004/35003

[commit-bot: I haz the power, 2012-04-26 02:58:47]

Change committed as 134055