1043488 - nsDocLoader fires multiple onLocationChange notifications for a single document load if the document is multipart

Status: NEW

(Core :: DOM: Navigation, defect)

Description

[:Gijs (he/him)]

Attachment: Stacks

STR:

1. set a breakpoint / add a console log to http://hg.mozilla.org/mozilla-central/annotate/616e6924cb0b/browser/base/content/browser.js#l4167

2. Load https://bugzilla.mozilla.org/buglist.cgi?quicksearch=localhost&list_id=10814105


ER:

your breakpoint/log is hit once

AR:
your breakpoint/log is hit multiple times


This is because bugzilla generates a multipart response.

I don't think onLocationChange should fire multiple times for these requests.

Practically speaking, the fact that it does breaks Real Things, like transient notification bars (bug 1041316).

Stacks for the relevant calls to FireOnLocationChange in DocLoader are attached.

I think what needs to happen is that CreateContentViewer needs to be taught that it shouldn't fire onLocationChange for the second-through-nth partial document loads (or perhaps we can just not create content viewers all the time? The fact that we're doing that seems odd, but I'm not that familiar with docshell).

While CreateContentViewer knows about whether it's a multipart document load, I don't know how to figure out if it's the first part or a later part. AFAICT, isLastPart is a lie at this point (it is always false in the breakpoints described above), and anyway, I should like to fire the notification as soon as possible, not as late as possible...

Boris/Neil, is this something you can help figure out?

Comment 1

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

I would expect FireOnLocationChange in this case, but perhaps we could pass some flag.
It wouldn't be LOCATION_CHANGE_SAME_DOCUMENT, but something like
LOCATION_CHANGE_MULTIPART

Comment 2

[Boris Zbarsky [:bzbarsky]]

From docshell's point of view these are all different documents.  You get different Document objects, different windows, some of them might not get handled by docshell at all (e.g. handled via helper apps), etc.

Basically, necko/uriloader converts a multipart response this to a bunch of separate channels that are loaded in the docshell one after the other.  See the multipart stream converter.

So here's the real question: in what way do you want to treat part of a multipart response differently from an HTML page that just sets location.href to a new URI in an inline script and why?

Comment 3

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #2)
> From docshell's point of view these are all different documents.  You get
> different Document objects, different windows, some of them might not get
> handled by docshell at all (e.g. handled via helper apps), etc.
> 
> Basically, necko/uriloader converts a multipart response this to a bunch of
> separate channels that are loaded in the docshell one after the other.  See
> the multipart stream converter.
> 
> So here's the real question: in what way do you want to treat part of a
> multipart response differently from an HTML page that just sets
> location.href to a new URI in an inline script and why?

I don't really follow. From the user's perspective, a multipart response appears as 1 webpage. Having an inline script that redirects to a second webpage involves two webpages, so it counts as 2 navigations.

If you're trying to make a deeper point about the fact that maybe the notifications code should be tracking something else, then, well, I guess so - but I don't think we have a way to distinguish user-initiated versus script-initiated loads (and the border is of course fuzzy anyway - <a href="setTimeout(() => location.href = 'foo.com', 5000)">click me!</a> ) , and in the meantime the fact that "one" webpage counts as "two" for this code is problematic...

Comment 4

[:Gijs (he/him)]

Hmm, trying to take fewer leaps of interpretation: the code really only cares when the docshell's toplevel URI (sans hash) changed. We already filter out same-document navigation with the appropriate flag, and non-toplevel navigations with the right property... but this scenario doesn't get filtered out.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> From the user's perspective, a multipart response appears as 1 webpage. 

You can have a multipart response where you have an HTML page, then 5 minutes later it becomes a PDF, then 5 minutes later it becomes a full-page video, then another HTML page, etc, forever.  I really doubt users would consider that "1 webpage".

What you mean is that there are _some_ multipart responses that appear as 1 webpage.  Just like there are some multiple-page HTML loads that appear as 1 webpage, because they do quick redirects.

> Having an inline script that redirects to a second webpage involves two webpages

How so?  The user never sees anything rendered for the page with the script!

Can you define what you mean by "one webpage"?

Comment 6

[Boris Zbarsky [:bzbarsky]]

> the code really only cares when the docshell's toplevel URI (sans hash) changed.

What happens when a page has a link to itself (but after setting document.cookie so that the response when it comes is totally different from the original page, if you want to add complications)?

Comment 7

[:Gijs (he/him)]

Fundamentally, I don't think there's a perfect solution to the "what is a single webpage" problem, because of how many options there are for page authors in general. Plenty of single-page web apps never change URIs (or change them with history push/pop, but never change the document that's loaded). Do those all appear as "the same webpage" to the user? Probably not.

It's a difficult problem because for notification bars like "This page showed a popup" or "The browser did X for this page, do you want to do Y instead" or "The browser could do Z, would you like it to?" - they're all tied to this notion of "single webpage", and should go away if you navigate.

We've not really had issues with the edgecases like multipart docs, <meta> refreshes, script-based location writing and so on, because most of what we used to show a notification bar for happened after the page had fully loaded (popups, translation, requesting permissions for web APIs, whatever).

Now that we have this bar which shows pretty much immediately when you hit enter in the location bar (ie before the page finishes loading), I guess we'll run into a lot of them (this might be the first because it affects bugzilla, but I wouldn't be surprised if there were search engines that used other techniques that have already been discussed).

I don't know what the perfect solution here is. While I understand that a multipart document *could* do what you describe (spend 5 minutes between docs), it sounds unlikely in practice... but then, we're living on the web... :-)

Comment 8

[:Gavin Sharp [email: [e-mail]]]

onLocationChange is about location changes, not document changes. That a multipart page keeps changing its content doesn't mean it keeps changing its location, so it doesn't make sense for onLocationChange to keep firing.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> it sounds unlikely in practice

It happens pretty often for multipart streams of jpegs, with delays that are in the seconds to minutes ranges.

I agree that having it happen for parts including HTML is not very likely.

> onLocationChange is about location changes

Then we should stop firing it when the user hits the reload button?

Comment 10

[Boris Zbarsky [:bzbarsky]]

But we _should_ fire it for a multipart if the user follows an internal anchor in one of the parts before the next part replaces it?

Comment 11

[:Gijs (he/him)]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > onLocationChange is about location changes
> 
> Then we should stop firing it when the user hits the reload button?

I think at this point I'd rather rename it to something that more closely reflects what it means, but that probably breaks the world. :-(

Comment 12

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #9)
> > onLocationChange is about location changes
> 
> Then we should stop firing it when the user hits the reload button?

Yes, that'd be ideal, and might even solve some janky UI behavior - but I don't think that kind of change is feasible given compatibility constraints. That the notification has one odd behavior isn't justification to avoid fixing another.

(In reply to Boris Zbarsky [:bz] from comment #10)
> But we _should_ fire it for a multipart if the user follows an internal
> anchor in one of the parts before the next part replaces it?

Fire it with LOCATION_CHANGE_SAME_DOCUMENT, yes. Internal anchors are part of the "location" conceptually.

Comment 13

[Boris Zbarsky [:bzbarsky]]

It's not SAME_DOCUMENT, though, in the sense of actually having the same Document.  I meant for the _next_ part, not for the actual scroll-to-anchor; sorry that was unclear.

Comment 14

[[e-mail]]

(In reply to Boris Zbarsky from comment #5)
> Can you define what you mean by "one webpage"?

How about "one top-level session history entry" as a heuristic? This would then cover the bugzilla search case, plus common redirect cases too.

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> It's not SAME_DOCUMENT, though, in the sense of actually having the same
> Document.  I meant for the _next_ part, not for the actual scroll-to-anchor;
> sorry that was unclear.

I see - certainly an edge case. IMO no notification should fire in that case.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> IMO no notification should fire in that case.

Except the URI goes from "foo#bar" to "foo".

Comment 17

[:Gavin Sharp [email: [e-mail]]]

Well then fire a notification - I don't particularly feel strongly about that edge case!