923331 - Stop creating nsISHistory object lazily

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: rm-disablehistory

We sort of create this object lazily, but there doesn't seem to be any benefit in doing so. It's created before we actually show the browser window in either case. And creating lazily forces us to do some pretty gross stuff in the session history code in case it hasn't been created yet.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: remove-history-wait

This patch has the session store cleanups.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 813395 [details] [diff] [review]
rm-disablehistory

>diff --git a/toolkit/content/widgets/browser.xml b/toolkit/content/widgets/browser.xml

>+                try {
>+                  this.docShell.useGlobalHistory = true;
>+                } catch(ex) {
>+                  Components.utils.reportError("Places database may be locked: " + ex);

This isn't a very good error message, let's take this opportunity to improve it:

// This can occur if the Places database is locked
Components.utils.reportError("Error enabling browser global history: " + ex);

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Thanks for fixing this!

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 813397 [details] [diff] [review]
remove-history-wait

Review of attachment 813397 [details] [diff] [review]:
-----------------------------------------------------------------

Great stuff, thanks!

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: docshell-getter

I ran into an additional problem when I tested this some more. If someone accesses gBrowser too early during window initialization, then we'll attach the browser.xml binding to the <browser> element before layout has happened. In that case, we'll run the constructor and the docshell will be null. Therefore, we'll never create the nsISHistory object.

In the case I saw, gBrowser was accessed by the new tab preloader, which touches gBrowser from a timer here:
http://mxr.mozilla.org/mozilla-central/source/browser/modules/BrowserNewTabPreloader.jsm#262
However, the access could just as easily happened from an addon or something, so we can't just fix the new tab preloader code.

I got some help from smaug on this, and he came up with a nice solution. We actually have a docshell available from the moment that the <browser> element is attached to the document. It's just that the code used by the XBL getter code for the "docShell" property does this:
  this.boxObject.QueryInterface(Components.interfaces.nsIContainerBoxObject).docShell
and that code only returns non-null if we've laid out the element. If we do this instead:
  this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader.docShell
then we'll get a docshell as long as the element is in a document.

One odd thing is that we need to null-check this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner).frameLoader because we apparently still execute XBL code event after elements have been removed from the document. I don't know why this is, but it seems to happen. However, the old code also would have returned null in that case, so there should be no change in behavior.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

That sounds a lot like bug 880101. You might want to steal the test from there as well :) Does this mean we could remove the workaround from bug 880101 maybe with this patch?

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/754cf7fc84cd
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/eba758f1fba3
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/dca0f18f3e86

I filed bug 927912 for the cleanups related to the docshell always being available at startup now. I'll try to get to it in the next couple days.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/754cf7fc84cd
https://hg.mozilla.org/mozilla-central/rev/eba758f1fba3
https://hg.mozilla.org/mozilla-central/rev/dca0f18f3e86