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

RESOLVED FIXED in Firefox 44

Status

()

Core
Document Navigation
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: MattN, Assigned: Gijs)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {testcase})

unspecified
mozilla44
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Created attachment 8663852 [details]
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.
Created attachment 8663857 [details]
Testcase entry-point (frame_pushstate_wpl.html) which frames the other file
(Assignee)

Comment 2

3 years ago
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).
(Assignee)

Comment 3

3 years ago
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...
(Assignee)

Comment 4

3 years ago
(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.
Summary: WebProgressListener's onLocationChange doesn't get notified for the first pushState in a frame → WebProgressListener's onLocationChange doesn't get notified for pushState calls in a frame that alter the URI
(Assignee)

Comment 5

3 years ago
(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. :-\
(Assignee)

Comment 6

3 years ago
Created 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
Attachment #8664794 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

3 years ago
(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?).
(Assignee)

Comment 8

3 years ago
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
(Assignee)

Comment 9

3 years ago
Ugh, this time including the actual test files:

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

Updated

3 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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...
Attachment #8664794 - Flags: review?(bzbarsky) → review?(bugs)
> 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 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.
Attachment #8664794 - Flags: review?(bugs) → review+
s/But we need a test here.//
(Assignee)

Comment 14

3 years ago
(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...)
(Assignee)

Comment 16

3 years ago
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.
(Assignee)

Updated

3 years ago
Attachment #8664794 - Attachment description: MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?bz → MozReview Request: Bug 1206879 - fire onlocationchange from pushState in frames when the URI changes, r?smaug
Attachment #8664794 - Flags: review+ → review?(bugs)
(Assignee)

Comment 17

3 years ago
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
(Assignee)

Comment 18

3 years ago
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.

Updated

3 years ago
Attachment #8664794 - Flags: review?(bugs) → review+
(Assignee)

Comment 19

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

for the isSubframe removal.
(Assignee)

Comment 21

3 years ago
(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
Flags: needinfo?(bugs)
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!
Flags: needinfo?(bugs)
(Assignee)

Updated

3 years ago
Depends on: 1209947
https://hg.mozilla.org/mozilla-central/rev/ba3339abba12
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.