1206879 - WebProgressListener's onLocationChange doesn't get notified for pushState calls in a frame that alter the URI

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Matthew N. [:MattN]]

Attachment: pushstate_wpl.html (testcase helper file loaded in a frame)

When a pushState occurs in an iframe, the first pushState doesn't trigger nsIWebProgressListener's onLocationChange. Future pushState calls do trigger onLocationChange.

This may be a dupe of bug 912513 but my testcase is without extra nesting of frames.

Comment 1

[Matthew N. [:MattN]]

Attachment: Testcase entry-point (frame_pushstate_wpl.html) which frames the other file

Comment 2

[:Gijs (he/him)]

The issue seems to be that the code here:

https://dxr.mozilla.org/mozilla-central/rev/a1ccea59e254a88f7bb44b0ad8a58b77b7eca339/docshell/base/nsDocShell.cpp#11564-11599

relies on SetCurrentURI to call OnLocationChange, in the case where the URI changed. It does not verify that this happens (it could do so by checking the return value).

Inside SetCurrentURI, we end up hitting this path:

https://dxr.mozilla.org/mozilla-central/rev/a1ccea59e254a88f7bb44b0ad8a58b77b7eca339/docshell/base/nsDocShell.cpp#1922-1929

  if (!isSubFrame && !isRoot) {
    /*
     * We don't want to send OnLocationChange notifications when
     * a subframe is being loaded for the first time, while
     * visiting a frameset page
     */
    return false;
  }

and fail to fire OnLocationChange. Because the push state is happening on a subframe, I would expect isSubFrame to be true here, and I don't know why it isn't. Looking at that next (though really, we might just need to check whether SetCurrentURI fired OnLocationChange or not (by using the rv) inside the AddState method).

Comment 3

[:Gijs (he/him)]

mLSHE is null in this case... and the repetitive calls trigger the other path inside the code I linked, where the URIs *are* equal. I'm going to see what happens if the pushState uses a different URI every time, but I need to modify the testcase for that...

Comment 4

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #3)
> mLSHE is null in this case... and the repetitive calls trigger the other
> path inside the code I linked, where the URIs *are* equal. I'm going to see
> what happens if the pushState uses a different URI every time, but I need to
> modify the testcase for that...

Right. When changing the URI, I never see an onlocationchange at all.

Looking at the nsDocShell.h docs, it seems:

  // Reference to the SHEntry for this docshell until the page is destroyed.
  // Somebody give me better name
  nsCOMPtr<nsISHEntry> mOSHE;
  // Reference to the SHEntry for this docshell until the page is loaded
  // Somebody give me better name.
  // If mLSHE is non-null, non-pushState subframe loads don't create separate
  // root history entries. That is, frames loaded during the parent page
  // load don't generate history entries the way frame navigation after the
  // parent has loaded does. (This isn't the only purpose of mLSHE.)
  nsCOMPtr<nsISHEntry> mLSHE;

it is quite likely that mLSHE will be null here, because the page is loaded (well, typically but not necessarily, I suppose, when pushState is called).

Seems like we should check isSubframe on the mOSHE pointer as well.

Comment 5

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #4)
> Seems like we should check isSubframe on the mOSHE pointer as well.

So it turns out that's just false, so that doesn't get us anywhere for this bug. :-\

Comment 6

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?bz

Comment 7

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #6)
> Created attachment 8664794 [details]
> MozReview Request: Bug 1206879 - fire onlocationchange from pushState in
> frames when the URI changes, r?bz
> 
> Bug 1206879 - fire onlocationchange from pushState in frames when the URI
> changes, r?bz

Try push: remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=abfd02ba47f0

Note that I've left in the mOSHE check because it seems to me like we should ensure that we do get the right subframe information, even if the document has already loaded. Of course, it could be that I misunderstand what the code is trying to do (in which case maybe we should add a comment?).

Comment 8

[:Gijs (he/him)]

Comment on attachment 8664794 [details]
MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?bz

Comment 9

[:Gijs (he/him)]

Ugh, this time including the actual test files:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=7264bb483423

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8664794 [details]
MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

Going to punt this one to Olli.... It looks like we almost never SetIsSubFrame on session history entries, which seems odd.  But also, I don't know whether the synthetic OnLocationChange is the right thing here...

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

> https://dxr.mozilla.org/mozilla-central/rev/
> a1ccea59e254a88f7bb44b0ad8a58b77b7eca339/docshell/base/nsDocShell.cpp#1922-
> 1929
> 
>   if (!isSubFrame && !isRoot) {
>     /*
>      * We don't want to send OnLocationChange notifications when
>      * a subframe is being loaded for the first time, while
>      * visiting a frameset page
>      */
>     return false;
>   }
> 
> and fail to fire OnLocationChange. Because the push state is happening on a
> subframe, I would expect isSubFrame to be true here, and I don't know why it
> isn't.
nsISHEntry.isSubFrame isn't about something being a frame, but about session history transaction created for
a subframe navigation. And because we're doing top level navigation here, it is false.



But it is not clear to me why the nsDocShell part of the patch (the 3rd attachment) for bug 82236 was right in any way.
IMO we should get OnLocationChange when, well, location does change. 
If we could get rid of GetIsSubFrame call in SetCurrentURI, we could remove nsISHEntry.isSubFrame. 
Could you perhaps push patch without nsISHEntry.isSubFrame to tryserver to see what all breaks?



To fix this bug, the change to ::AddState looks ok to me, but please don't change SetCurrentURI

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8664794 [details]
MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

So r+ for the ::AddState change (even though it is a bit silly, but fixing SetCurrentURI is scary).

But we need a test here.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

s/But we need a test here.//

Comment 14

[:Gijs (he/him)]

(so there are issues with this existing try push and I need to make some time to (a) do a trypush with just the test + the AddState change, and (b) to do a trypush Olli suggested - separately...)

Comment 15

[:Gijs (he/him)]

just addstate + test:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eef6994fc48

Comment 16

[:Gijs (he/him)]

So, my patch is just bogus. SetCurrentURI always returns false when you pass in |true|, so we always end up firing a (sometimes second) locationchange event, which the test catches. Yay automated tests.

As to what the right patch here is, well... there are several options.

Comment 17

[:Gijs (he/him)]

Comment on attachment 8664794 [details]
MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug

Comment 18

[:Gijs (he/him)]

This is kind of clownshoes, but it does work (locally, at least)...

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=565c6e3410f9


I'll do the requested "kill the subframe thing" trypush in a second.

Comment 19

[:Gijs (he/him)]

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a1d43abb1c

for the isSubframe removal.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/ba3339abba12

Comment 21

[:Gijs (he/him)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Could you perhaps push patch without nsISHEntry.isSubFrame to tryserver to
> see what all breaks?

> https://treeherder.mozilla.org/#/jobs?repo=try&revision=c8a1d43abb1c

"quite a bit, but not everything", it seems. Some of the identity box stuff looks concerning, though.

Do you want me to file a followup bug to actually do this?

remote:   https://hg.mozilla.org/integration/fx-team/rev/ba3339abba12

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

That looks quite isolated issues, or how to say. And better than I was afraid of.
Please file a followup bug yes. I might try to look at it if I find time during next Q.
Or feel free to take that bug ;)


Thanks for doing that try-push!

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ba3339abba12