Stop creating nsISHistory object lazily

RESOLVED FIXED in Firefox 27

Status

()

Firefox
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

Trunk
Firefox 27
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Created attachment 813395 [details] [diff] [review]
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.
Attachment #813395 - Flags: review?(gavin.sharp)
(Assignee)

Comment 1

4 years ago
Created attachment 813397 [details] [diff] [review]
remove-history-wait

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
(Assignee)

Comment 5

4 years ago
Created attachment 814901 [details] [diff] [review]
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.
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+
(Assignee)

Comment 7

4 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/754cf7fc84cd
https://hg.mozilla.org/mozilla-central/rev/eba758f1fba3
https://hg.mozilla.org/mozilla-central/rev/dca0f18f3e86
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27

Updated

4 years ago
Depends on: 931399
Depends on: 927912
Depends on: 971685
You need to log in before you can comment on or make changes to this bug.