1318126 - Improve prefetch

Status: NEW

(Core :: Networking, defect, P5)

Description

[Dennis Schubert [:denschub]]

When testing for prefetch support on [0] or [1], the result says prefetching is not supported in Firefox. If looking at the developer tools, one can see that the requests get canceled immediately

When running with `MOZ_LOG=nsPrefetch:5`, I can see that the request is started but cancelled immediately:

> [Main Thread]: D/nsPrefetch ProcessNextURI [http://browserspy.dk/prefetch.php?pf=669492067]
> [Main Thread]: D/nsPrefetch ProcessNextURI [http://browserspy.dk/proxy.php]
> [Main Thread]: D/nsPrefetch StopPrefetching [stopcount=1]
> [Main Thread]: D/nsPrefetch done prefetching [status=804b0002]
> [Main Thread]: D/nsPrefetch done prefetching [status=804b0002]

and I am not able to determinate why exactly the request get canceled. Maybe, this is related to bug 1251573?

I was able to confirm that prefetching works in general when I build a minimal page and start it in an otherwise totally empty Firefox instance, but if I run my example in an instance with other tabs open, I can't see any prefetching. Maybe some limits are off here?

[0]: http://browserspy.dk/prefetch.php
[1]: http://www.browserscope.org/network/test

Comment 1

[Valentin Gosu [:valentin] (he/him)]

Based on the log of uriloader/prefetch/ I suspect either bug 1268962 or bug 580313.
Do you think you could help us track it down using mozregression? https://harthur.github.io/mozregression/

Comment 2

[Dragana Damjanovic [:dragana]]

taking it, I was the last one working on this.

Comment 3

[Dragana Damjanovic [:dragana]]

[Main Thread]: D/nsPrefetch StopPrefetching [stopcount=1]

StopPrefetching is called when a document load starts:
https://dxr.mozilla.org/mozilla-central/source/uriloader/prefetch/nsPrefetchService.cpp#829


We do prefetch only if there is no document load. The prefetch will start the load of the document ends and will be canceld as soon as load of another document starts.
This was designed in this way from the beginning.

Comment 4

[Dragana Damjanovic [:dragana]]

Can you change pref network.prefetch-next.aggressiv and test again, this should not cancel the prefetch. This should confirm the comment 3.

Comment 5

[Samael Wang [:freesamael] (away for now)]

I think we can slightly improve it by ignoring loads of about and wyciwyg URIs in nsPrefetchService::OnStateChange, but it doesn't help for this specific case. The prefetching would still be canceled by a load to google ads.

Comment 6

[Boris Zbarsky [:bzbarsky]]

The idea, of course, is that prefetch should not slow down pageloads...

Comment 7

[Dragana Damjanovic [:dragana]]

I would close this bug as invalid or rename it "Improve prefetch in case of only about: and wyciwyg URIs loads". maybe also in case of loading ads.

Comment 8

[Dennis Schubert [:denschub]]

Yeah, setting `network.prefetch-next.aggressive` to true indeed forces the prefetch to occur. Thanks for renaming this issue, Dragana.

(clearing the ni? since there is no regression.)

Comment 9

[Boris Zbarsky [:bzbarsky]]

Wait.  Why are we special-casing about: or wyciwyg:?  Those aren't cases that matter in practice, outside of the silly test suites, right?  We shouldn't be "gaming" the test suites like that, imo.

What does the spec say for the behavior here?  What behavior would be most useful on the web in general?  _Those_ are the things we should get figured out and then adjust our implementation accordingly.

Comment 10

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> Wait.  Why are we special-casing about: or wyciwyg:?  Those aren't cases
> that matter in practice, outside of the silly test suites, right?  We
> shouldn't be "gaming" the test suites like that, imo.
> 
> What does the spec say for the behavior here?  What behavior would be most
> useful on the web in general?  _Those_ are the things we should get figured
> out and then adjust our implementation accordingly.

Sorry, the title was wrong, I was more thinking about loads like ads, trackers and such.

When I thing about it, and I just checked, the trigger for stopping prefetch is a document load, so that is good enough.
I will close the bug.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Wait, what?

It seems like we have a problem: our prefetch setup is completely useless in practice.  What's the bug that tracks fixing that?

Comment 12

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #11)
> Wait, what?
> 
> It seems like we have a problem: our prefetch setup is completely useless in
> practice.  What's the bug that tracks fixing that?

I have not implemented the current version, it is probably based on some old draft.
 
I have just scan through the last draft, I have not read one since very long time. Some part change. The behavior is not really strictly define, a lot of may, but also should not block, etc. (this was really quick read) 

We can open another bug(s) for this, there are multiple aspects that can be change.

Necko's current activities will improve prefetch behavior but that will come later not now (we have that in our notes).

I think the bigger thing is actually part of dom and I am not right person to work on that. e.g. when a new navigation starts decide if the resource being prefetched is important for this navigation or if it is part of another navigation, cancel prefetch if user is navigating on another tab, etc.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> We can open another bug(s) for this

Ignoring for the moment that we _have_ a perfectly reasonable bug for it (this one, at least until we ended up discussing whether it's the right bug)... have you filed those other bugs?  Or is it someone else's responsibility to make sure this issue doesn't just get buried because you decided to mark this bug invalid and move on?

Again, our current prefetch design means that prefetch doesn't actually happen in practice on real websites, as far as I can see.  That's not useful; we should either remove prefetch and all its complexity altogether or make it at least somewhat useful.  We need something to track fixing this; I don't care too much which team's turf you feel it should be on.

Comment 14

[Dragana Damjanovic [:dragana]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #13)
> > We can open another bug(s) for this
> 
> Ignoring for the moment that we _have_ a perfectly reasonable bug for it
> (this one, at least until we ended up discussing whether it's the right
> bug)... have you filed those other bugs?  Or is it someone else's
> responsibility to make sure this issue doesn't just get buried because you
> decided to mark this bug invalid and move on?
> 
> Again, our current prefetch design means that prefetch doesn't actually
> happen in practice on real websites, as far as I can see.  That's not
> useful; we should either remove prefetch and all its complexity altogether
> or make it at least somewhat useful.  We need something to track fixing
> this; I don't care too much which team's turf you feel it should be on.

Necko bug will be file when the other parts are ready, currently the stuff I had in mind are not possible.

I will reopen this one.

Comment 15

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

This is a P1 bug without an assignee. 

P1 are bugs which are being worked on for the current release cycle/iteration/sprint. 

If the bug is not assigned by Monday, 28 August, the bug's priority will be reset to '--'.

Comment 16

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 17

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 18

[Jens Stutte [:jstutte]]

Bulk-downgrade of unassigned, >=3 years untouched DOM/Storage bug's priority.

If you have reason to believe this is wrong, please write a comment and ni :jstutte.

Comment 19

[Ksenia Berezina [:ksenia]]

Adding ni for Dennis to see if this is still an issue.

Comment 20

[Dennis Schubert [:denschub]]

Yes, it is.