Closed Bug 971697 Opened 6 years ago Closed 6 years ago

Let the frameLoader take care of creating nsISHistory

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox27 --- wontfix
firefox28 --- wontfix
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

References

Details

Attachments

(1 file, 3 obsolete files)

In SessionStore upon loading the frame script we do:

  let webNavigation = docShell.QueryInterface(Ci.nsIWebNavigation);
  webNavigation.sessionHistory...

Due the to frame script being added to the window listener we are mostly loaded before .sessionHistory is set. This doesn't seem to be a problem in real e10s but it is today.

DocShell::SetSessionHistory() should send a notification like "sessionhistory-available" that we can listen for and then start doing our thing.

OTOH, can we maybe ensure that just like for e10s shistory has already been set? For non-remote browser we just let the xul:browser constructor create and set shistory. I couldn't figure out how e10s does it, though.

Bill, can you maybe shed some light onto this? I see that toolkit/content/browser-child.js does the same as content-sessionStore.js, just QI to nsIWebNavigation and access the .sessionHistory property but there must be some code that sets it, no?
Flags: needinfo?(wmccloskey)
Whiteboard: p=0
In TabChild::Init [1] we create an nsWebBrowser. That's what holds the docshell tree in the child process, since we doesn't have a frameloader there. Then in nsWebBrowser::Create we call SetSessionHistory [2]. I'm not exactly sure who calls nsWebBrowser::Create, but presumably it happens somewhere in TabChild::Init. I don't have access to gdb just now or I'd check.

Are you seeing any cases where we don't have the history available? It seems like we create it pretty early in both cases.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#642
[2] http://mxr.mozilla.org/mozilla-central/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1206
Flags: needinfo?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #1)
> Are you seeing any cases where we don't have the history available? It seems
> like we create it pretty early in both cases.

When calling SessionHistory.isEmpty() from the frame script when it is loaded, webNavigation.sessionHistory is always empty when opening a new tab. I just checked by putting some dump() statements in there. This doesn't happen in e10s, presumably because the nsWebBrowser takes care of setting .sessionHistory.
The xul:browser constructor is executed after the frame script is loaded. If the constructor sets .sessionHistory then I'm not sure what to do about that.
Oh, hmm, right. I wonder if we could just have the frameloader create the nsSHistory object. I don't think there's any reason why the frontend should have to deal with stuff like this. Olli, does that seem okay to you?
Flags: needinfo?(bugs)
How's this? Ensured that this makes webNavigation.sessionHistory not be null on frame script load.
Attachment #8375007 - Flags: review?(wmccloskey)
Attachment #8375007 - Flags: review?(bugs)
Comment on attachment 8375007 [details] [diff] [review]
0001-Bug-971697-Let-the-frameLoader-take-care-of-creating.patch

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

Looks good to me, but I'll just give f+ since I'm not a peer for this code.

::: toolkit/content/widgets/browser.xml
@@ +747,5 @@
>  
>        <constructor>
>          <![CDATA[
>            try {
> +            if (this.webNavigation.sessionHistory) {

Could you please add a comment to say that:
1. this.docShell should always be non-null here.
2. this.webNavigation.sessionHistory should be non-null as long as !this.hasAttribute("disablehistory").

@@ +756,2 @@
>                // enable global history if we weren't told otherwise
>                if (!this.hasAttribute("disableglobalhistory") && !this.isRemoteBrowser) {

I guess this first test hasn't been useful for a while.
Attachment #8375007 - Flags: review?(wmccloskey) → feedback+
(In reply to Bill McCloskey (:billm) from comment #6)
> @@ +756,2 @@
> >                // enable global history if we weren't told otherwise
> >                if (!this.hasAttribute("disableglobalhistory") && !this.isRemoteBrowser) {
> 
> I guess this first test hasn't been useful for a while.

Wait, you mean we should remove checking for the disableglobalhistory attribute? That is still used by code. Or am I misunderstanding your comment?
Oops, sorry. I confused disablehistory with disableglobalhistory.
Comments added.
Attachment #8375196 - Flags: review?(bugs)
Attachment #8375196 - Flags: feedback+
Attachment #8375007 - Attachment is obsolete: true
Attachment #8375007 - Flags: review?(bugs)
Comment on attachment 8375007 [details] [diff] [review]
0001-Bug-971697-Let-the-frameLoader-take-care-of-creating.patch

What about xul:editor or xul:iframe? Wouldn't they session history with this setup
Attachment #8375007 - Attachment is obsolete: false
Flags: needinfo?(bugs)
Attachment #8375196 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #10)
> What about xul:editor or xul:iframe? Wouldn't they session history with this
> setup

Very good point. This patch now only does it for xul:browser. Would you favor a solution where we only create and set shistory if a specific attribute is present? Something like <xul:browser withsessionhistory="true"/>?
Attachment #8375007 - Attachment is obsolete: true
Attachment #8375196 - Attachment is obsolete: true
Attachment #8375209 - Flags: review?(bugs)
Comment on attachment 8375209 [details] [diff] [review]
0001-Bug-971697-Let-the-frameLoader-take-care-of-creating.patch, v3

I think this is fine, if this is fine to toolkit peers.

You can do mOwnerContent->IsXUL(nsGkAtoms::browser)
Attachment #8375209 - Flags: review?(bugs) → review+
Ok, let's ask the toolkit owner.
Assignee: smacleod → ttaubert
Attachment #8375209 - Attachment is obsolete: true
Attachment #8375220 - Flags: review?(dtownsend+bugmail)
Summary: Send a notification when the session history has been set → Let the frameLoader take care of creating nsISHistory
Comment on attachment 8375220 [details] [diff] [review]
0001-Bug-971697-Let-the-frameLoader-take-care-of-creating.patch, v4

I think this is a nicer way to do this
Attachment #8375220 - Flags: review?(dtownsend+bugmail) → review+
Backed out for m-oth orange on Windows and Linux:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/chrome/content/base/test/chrome/test_cpows.xul | Error while executing:
TypeError: this.docShell is null

https://hg.mozilla.org/integration/fx-team/rev/e4ec0b12cdb3
Looks like the failures were only caused by removing the |this.docShell| check from the xul:browser constructor.
Ok, we should keep the |this.docShell| check as the "remote-browser" binding inherits from "browser" (and thus inherits the constructor) and remote browsers have no docShells. Looks like destructors aren't inherited but the tabbrowser calls browser.destroy() explicitly so we need to put the check back in there as well.
(In reply to Tim Taubert [:ttaubert] from comment #18)
> Looks like destructors aren't inherited but the
> tabbrowser calls browser.destroy() explicitly so we need to put the check
> back in there as well.

To be exact, the destructor didn't have a check before but it now needs one.
https://hg.mozilla.org/mozilla-central/rev/9765f6d4cad5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Comment on attachment 8375220 [details] [diff] [review]
0001-Bug-971697-Let-the-frameLoader-take-care-of-creating.patch, v4

[Approval Request Comment]
Bug caused by (feature/regressing bug #): No regression.
User impact if declined: None.
Testing completed (on m-c, etc.): On Nightly, m-c, and inbound without problems so far.
Risk to taking this patch (and alternatives if risky): Low risk.
String or IDL/UUID changes made by this patch: None.

I would really like to uplift this patch to help us fix the regression in bug 967028. It makes writing this patch a lot easier if we don't have to awkwardly poll for webNavigation.sessionHistory to become available but can just rely on it being there when the frame script is loaded.
Attachment #8375220 - Flags: approval-mozilla-aurora?
Attachment #8375220 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
No longer blocks: fxdesktopbacklog
Whiteboard: p=0
Since Tim is on PTO, could someone else please provide STR in order to verify this fix?
I don't think it's useful to try to verify this directly, the failure case is "session history stops working", and we've got automated test coverage for that.
You need to log in before you can comment on or make changes to this bug.