Closed Bug 1150200 Opened 9 years ago Closed 9 years ago

Can't navigate back to previous page sometimes

Categories

(Firefox :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 40
Tracking Status
e10s m6+ ---
firefox39 --- unaffected
firefox40 --- fixed

People

(Reporter: ttaubert, Assigned: mconley)

References

(Depends on 2 open bugs)

Details

(Keywords: regression)

Attachments

(2 files)

STR:

1) Open reddit
2) Click a few links
3) Two-finger swipe left to go back to the reddit main page every time
   (Clicking the back button doesn't work either.)

This doesn't happen all the time, usually takes a few tries to find a link you can't return from.

Git bisect tells me it's caused by bug 1137933.
I think this is because the docShell used to be QI'd to an nsIWebNavigation in the initialization of the WebNavigation object. This now only occurs in the getter of webNavigation.

So anything that was using docShell.canGoForward or docShell.canGoBack is going to not work.

We should probably change browser-child.js to use this.webNavigation instead of docShell.
tracking-e10s: --- → ?
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #1)
> We should probably change browser-child.js to use this.webNavigation instead
> of docShell.

It's doing that already:

https://hg.mozilla.org/mozilla-central/rev/00e022f7e45e
Oh, hrm, I see... we get this.webNavigation in the initter function. Weird...
Oh, right. Missed those places. So from some more investigation it seems like RemoteWebNavigation isn't even sending messages anymore when we get "stuck".
Well that's probably because we send an onLocationChange message with canGoBack=false.
Yeah, docShell.canGoBack is undefined in those cases.
Comment on attachment 8587007 [details] [diff] [review]
QI the docShell to an nsIWebNavigation before sending canGoForward or canGoBackward in browser-child.js. r=?

So this fixes things for me. Working on a regression test separately.
Attachment #8587007 - Flags: review?(ttaubert)
While this should fix it, docShell is indeed missing the nsIWebNavigation interface, I wonder what actually happens here. It's the same docShell and the same frame script, we're just navigating to different pages. I would be really interested to know why docShell suddenly "loses" an interface...
Comment on attachment 8587007 [details] [diff] [review]
QI the docShell to an nsIWebNavigation before sending canGoForward or canGoBackward in browser-child.js. r=?

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

Whatever the cause is, the new code is certainly more correct.
Attachment #8587007 - Flags: review?(ttaubert) → review+
Thanks for fixing this guys!

I guess if the JS wrapper around the docshell is GCed (which is possible with my patch and wasn't before) then whatever interfaces were on the JS wrapper will be lost. The next time we access the docshell from JS, we would create a new wrapper without nsIWebNavigation on it. I don't know if that's what's happening, but it seems possible.
That makes a lot of sense. I thought that something got GCed due to the different objects we're holding onto with the new patch now but wasn't aware that only the JS wrappers can be GCed. Thanks for shedding some light!
Strangely, my test leaks windows on shutdown when running with non-e10s... not sure why. I'll investigate tomorrow.

remote:   https://hg.mozilla.org/integration/fx-team/rev/122413f5b051
Whiteboard: [please leave open]
Keywords: leave-open
Whiteboard: [please leave open]
Blocks: 1150099
No longer blocks: 1150099
Depends on: 1150610
Depends on: 1150643
This bug is fixed, but we haven't been able to land the regression test because of bug 1150643.

I'm going to close this one out, and file a new bug to get the test landed once bug 1150643 is fixed.
Depends on: 1153414
Ok, filed bug 1153414 to land the test.
Status: NEW → UNCONFIRMED
Ever confirmed: false
Target Milestone: --- → Firefox 40
uh, whoops.
Status: UNCONFIRMED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: