Closed Bug 923331 Opened 6 years ago Closed 6 years ago

Stop creating nsISHistory object lazily

Categories

(Firefox :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 27

People

(Reporter: billm, Assigned: billm)

References

Details

Attachments

(3 files)

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.
Attachment #813395 - Flags: review?(gavin.sharp)
This patch has the session store cleanups.
Attachment #813397 - Flags: review?(ttaubert)
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);
Attachment #813395 - Flags: review?(gavin.sharp) → review+
Thanks for fixing this!
Component: Session Restore → General
Comment on attachment 813397 [details] [diff] [review]
remove-history-wait

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

Great stuff, thanks!
Attachment #813397 - Flags: review?(ttaubert) → review+
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
Attached patch docshell-getterSplinter Review
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.
Attachment #814901 - Flags: review?(gavin.sharp)
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?
Attachment #814901 - Flags: review?(gavin.sharp) → review+
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.
Depends on: 931399
You need to log in before you can comment on or make changes to this bug.