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

NEW
Unassigned

Status

()

4 years ago
4 years ago

People

(Reporter: Gijs, Unassigned)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
Created attachment 8461676 [details]
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?
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
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?
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 3

4 years ago
(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...
Flags: needinfo?(gijskruitbosch+bugs)
(Reporter)

Comment 4

4 years ago
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.
> 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"?
Flags: needinfo?(gijskruitbosch+bugs)
> 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)?
(Reporter)

Comment 7

4 years ago
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... :-)
Flags: needinfo?(gijskruitbosch+bugs)
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.
> 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?
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?
(Reporter)

Comment 11

4 years ago
(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. :-(
(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.
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.
(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.
(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.
> IMO no notification should fire in that case.

Except the URI goes from "foo#bar" to "foo".
Well then fire a notification - I don't particularly feel strongly about that edge case!
You need to log in before you can comment on or make changes to this bug.