1150200 - Can't navigate back to previous page sometimes

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 1

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

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.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

(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

Comment 3

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

But not here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js?from=browser-child.js#135

or here: https://dxr.mozilla.org/mozilla-central/source/toolkit/content/browser-child.js?from=browser-child.js#136

Whipping up a patch.

Comment 4

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Oh, hrm, I see... we get this.webNavigation in the initter function. Weird...

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Oh, right. Missed those places. So from some more investigation it seems like RemoteWebNavigation isn't even sending messages anymore when we get "stuck".

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Well that's probably because we send an onLocationChange message with canGoBack=false.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Yeah, docShell.canGoBack is undefined in those cases.

Comment 8

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: QI the docShell to an nsIWebNavigation before sending canGoForward or canGoBackward in browser-child.js. r=?

Comment 9

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

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 11

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 12

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

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.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

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!

Comment 14

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Attachment: Regression test. r=?

Comment 15

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

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

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/122413f5b051

Comment 21

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

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.

Comment 22

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

Ok, filed bug 1153414 to land the test.

Comment 23

[Mike Conley (:mconley) (:⚙️) (PTO: May 27-31)]

uh, whoops.

Comment 24

[Firefox Bug Husbandry Bot]

Removing leave-open keyword from resolved bugs, per :sylvestre.