Closed Bug 580313 Opened 10 years ago Closed 4 years ago

prefetch links not followed in mutated HTML

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: gavinp, Assigned: dragana)

References

()

Details

Attachments

(2 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_8; en-US) AppleWebKit/533.4 (KHTML, like Gecko) Chrome/5.0.375.99 Safari/533.4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.8) Gecko/2009032608 Firefox/3.0.8 GTB7.1

If javascript in a page mutates the HTML, then no <link rel=prefetch ...> elements are respected.

Reproducible: Always

Steps to Reproduce:
1. Start mozilla.  Clear your cache.
2. Navigate to http://www.deepsky.com/~gpeters/prefetch-xkcd.html
3. Click on the button a few times.
4. Open a new tab, navigate to about:cache, then click on list of cached entries.
5. WHERE ARE MY COMIX?
Actual Results:  
No comics in cache.

Expected Results:  
Yes comics in cache.  The new link elements should have resulted in prefetches.
Prefetch is handled in the parser (so only at parse time), not in the DOM, iirc.  Nothing to do with networking.

I'm not sure we actually want to change this behavior, fwiw.
Component: Networking → DOM
QA Contact: networking → general
I filed this bug in part because my chrome implementation of this feature lacks this behaviour; so it will be a difference between Chrome and Firefox after we land prefetching.
(In reply to comment #1)
> Prefetch is handled in the parser (so only at parse time), not in the DOM,
> iirc.  Nothing to do with networking.
> 
> I'm not sure we actually want to change this behavior, fwiw.

Firefox respects dynamic changes / additions of LINK tags in other cases.  For example, you can remove/add a LINK tag to change the favicon dynamically.  It may be reasonable to handle rel=prefetch similarly.  Although, prefetching just happens at page load time, so if LINKs were added after page load time, it would be necessary to re-engage the prefetch system to process the new LINKs.  Hmm...
Status: UNCONFIRMED → NEW
Ever confirmed: true
> so it will be a difference between Chrome and Firefox 

That's ok, for things like prefetching, I would think.

I agree that most other links work dynamically (though dns-prefetch does not).  Most of them also _stop_ having an effect once you remove them, which is not something that prefetch can do.  So there's a fundamental difference there...

I wouldn't be opposed to changing the behavior, necessarily; it's just not clear to me that we want to.
Internet Explorer 11 honors DOM-appended prefetch <link>s. See my bug, 1025555 for testcases.
Google is rolling out features that require dynamic prefetch links to work[0]. Seems neat. Can we revisit this?

[0] https://plus.google.com/+IlyaGrigorik/posts/ahSpGgohSDo
re comment 6 and google search using it..
Flags: needinfo?(bzbarsky)
See comment 4 last paragraph.

Note that the precise behavior described in the link from comment 6 would disable bfcache in the form it currently exists in, so just naively implementing without adjusting the bfcache implementation may or may not be a good idea.
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] from comment #8)
> See comment 4 last paragraph.
> 
> Note that the precise behavior described in the link from comment 6 would
> disable bfcache in the form it currently exists in, so just naively
> implementing without adjusting the bfcache implementation may or may not be
> a good idea.

So the search results page would not be in bfcache at all? That certainly sounds bad. Can you elaborate a bit on what's going on there?
> So the search results page would not be in bfcache at all? 

Yes.

> Can you elaborate a bit on what's going on there?

Sure.  Bfcache only happens when we can precisely store the state of the page at the moment it was unloaded.  If there are in-flight network requests at that point, other than the one for the new document being loaded, we don't bfcache.  See nsDocument::CanSavePresentation the part that enumerates the loadgroup.

Of course if we moved to loadgroup-per-document and put the prefetches into the loadgroup for the new document (is that what we really want?) that would help things.  Or if we ignored the prefetches in CanSavePresentation, or something.
(In reply to Boris Zbarsky [:bz] from comment #10)
> Sure.  Bfcache only happens when we can precisely store the state of the
> page at the moment it was unloaded.  If there are in-flight network requests
> at that point, other than the one for the new document being loaded, we
> don't bfcache.  See nsDocument::CanSavePresentation the part that enumerates
> the loadgroup.

Cool, makes sense.

> 
> Of course if we moved to loadgroup-per-document and put the prefetches into
> the loadgroup for the new document (is that what we really want?) that would
> help things.  Or if we ignored the prefetches in CanSavePresentation, or
> something.

Since prefetch requests are really not part of the state of the document (IMHO), I think we can safely ignore them in CanSavePresentation().
Maybe we need a bit different setup here. If a new prefetch link is added to a document, we start loading it if the current docshell starts to load something from the same domain as the prefetch.
(Keeping prefetch to load something from a different domain feels wrong).
(In reply to Olli Pettay [:smaug] from comment #12)
> (Keeping prefetch to load something from a different domain feels wrong).

I don't know, I think it's relatively common to load resources from another domain. CDNs, etc. That said, this Google thing is a bit of a hack.
It is and we need some kind of spec for this, so that web devs and implementors could know how this all should work, at least in theory.
(In reply to Olli Pettay [:smaug] from comment #14)
> It is and we need some kind of spec for this, so that web devs and
> implementors could know how this all should work, at least in theory.

As luck would have it, such thing exists. It's Resource Hints: http://w3c.github.io/resource-hints/

That said, I also have an (editorial) proposal on the table to split above spec into two:
http://lists.w3.org/Archives/Public/public-web-perf/2014Dec/0020.html

The updated version of the RH spec documents the reactive use case:
https://cdn.rawgit.com/w3c/resource-hints/e19f621dad9856a92bf43a06a62489c0058d19de/index.html#reactive-resource-prefetching-preload
So as I reported offline to Ilya, the spec needs some clarifications on what the 
"Reactive resource prefetching" actually is and how it should work.
As of now one would just need to reverse engineer what blink does; in which cases does it start
prefetching and in which case not. (Does it matter if the <link> is in document or isn't, or if we're in a click event handler, like the spec hints etc.)

(I'm also still somewhat concerned that people might use 'reactive prefetching' as a replacement for 'ping' or sendBeacon. But maybe I shouldn't be... not sure yet.)
From my testing both trident and blink require the link to be in the document.

Additionally I think that FF should respect scripted "prefetch" links. From the rationale of this feature described in the mozilla FAQ:

> It is important that websites adopt <link> tag based prefetching instead of trying to roll-in silent downloading using various JS/DOM hacks. The <link> tag gives the browser the ability to know what sites are up to, and we can use this information to better prioritize document prefetching.

I thought this would be a great feature to simply request a prefetch and let the browser decide if/whether pre-fetching is the right thing to do or not (no metered/paid bandwidth, user preference etc.) and when to do the actual request. Additionally in case of multiple prefetch requests organize a separate low priority download queue etc..

Actually it would be nice to give us a simple JS API for this. But unless link mutations aren't observed, I need to rely on JS hacks.

This said the link[rel="prefetch"] should also fire a load event.
Alexander, did you make sure to clear your cache?

The following test case works for me in IE11, Chrome 39 and Chrome 41 Canary (But not FF 34, 35, 36b1 or Nightly 2015-01-19):
http://htmlpad.org/JsLinkRelPrefetch/

Expected behavior is to see requests for:
https://www.blogger.com/static/v1/widgets/2435745617-widget_css_bundle.css
http://apis.google.com/js/plusone.js
http://img1.blogblog.com/img/icon18_wrench_allbkg.png


I was able to reproduce (In reply to Alexander Farkas from comment #18)
> From my testing both trident and blink require the link to be in the
> document.
> 
> Additionally I think that FF should respect scripted "prefetch" links. From
> the rationale of this feature described in the mozilla FAQ:
> 
> > It is important that websites adopt <link> tag based prefetching instead of trying to roll-in silent downloading using various JS/DOM hacks. The <link> tag gives the browser the ability to know what sites are up to, and we can use this information to better prioritize document prefetching.
> 
> I thought this would be a great feature to simply request a prefetch and let
> the browser decide if/whether pre-fetching is the right thing to do or not
> (no metered/paid bandwidth, user preference etc.) and when to do the actual
> request. Additionally in case of multiple prefetch requests organize a
> separate low priority download queue etc..
> 
> Actually it would be nice to give us a simple JS API for this. But unless
> link mutations aren't observed, I need to rely on JS hacks.
> 
> This said the link[rel="prefetch"] should also fire a load event.
@hysistia@gmail.com

There is no contradiction to your testcase. I can confirm that trident and blink support scripted prefetch links. What I meant was, that those links needs to be inserted into the document. This was a question in comment above (https://bugzilla.mozilla.org/show_bug.cgi?id=580313#c16)
I would also like this to be addressed. As has been stated above, dynamic injection of other link tags fetches the associated asset.

I've written a library that will prefetch an asset when a user hovers over a link for n milliseconds. It works great in Chrome, but Firefox users don't see a perf gain from it. I've even attempted to create an iframe with the prefetch link inside of it, then inject it into the dom. Firefox even ignores this use of prefetch links despite the fact that it will parse all other link types in the iframe.

Not allowing dynamic link prefetching assumes devs know 100% of the links their users will need prefetched. This is a flawed assumption, as it is unlikely to provide users with the actual assets they will need prefetched. User interaction is one of the best indicators, along with user metrics, of what assets the user will need.

Please correct this behavior.
Dragana, interested to take this?

My concern from comment 16 is still something to figure out.
Flags: needinfo?(dd.mozilla)
(In reply to Olli Pettay [:smaug] from comment #22)
> My concern from comment 16 is still something to figure out.
Though, I guess it isn't much different to misusing cross-domain image loads  or script load etc.
I will take it.
Flags: needinfo?(dd.mozilla)
Assignee: nobody → dd.mozilla
Status: NEW → ASSIGNED
The first version is finished, but there is a couple of thing that we need to decide upon:
1) This patch does not do prerander (<link rel="preload" loadpolicy="next"...), instead it does just prefetch. I will need some help with that (I am not really familiar with dom but willing to learn it). And we need to decide how to load-balance that (maybe there is something for that).

2) I added logic to cancel prefetch, but we need to decide how we want this to look like: now only first request for a uri will start a prefetch and only from that document the prefetch can be cancelled. Do we want possibility that a prefetch can be started from multiple doc, but all this docs need to cancel it for the prefetch to be cancelled?  I think we need that, e.g. for fonts.

3) Our prefetch and preconnect logic does not have a notion of probability, we could add that (maybe opening separate bug for that)

4) We prefetch only when load of all docs is finished (nothing is loading) and if load of a doc starts we cancel and delete all prefeteches currently in progress or queued. A scenario: you click on a link and a script adds a <link> for prefetching some resource that that the new page will need. This scenario will not work with current prefetch service  because the prefetch will  be cancelled as soon as the new load starts. This will need more rework to decide which prefatch to keep and which to cancel.
Attached patch bug_580313_v1.patch (obsolete) — Splinter Review
also see previous comment.
Attachment #8710064 - Flags: feedback?(bugs)
(In reply to Dragana Damjanovic [:dragana] from comment #25)
> The first version is finished, but there is a couple of thing that we need
> to decide upon:
> 1) This patch does not do prerander (<link rel="preload"
> loadpolicy="next"...), instead it does just prefetch. I will need some help
> with that (I am not really familiar with dom but willing to learn it). And
> we need to decide how to load-balance that (maybe there is something for
> that).
>
Not sure what this is about. prerander? you mean prerender but that is all different.
and what is loadpolicy with preload? I don't see that in 
https://w3c.github.io/resource-hints/

 
> 2) I added logic to cancel prefetch, but we need to decide how we want this
> to look like: now only first request for a uri will start a prefetch and
> only from that document the prefetch can be cancelled. Do we want
> possibility that a prefetch can be started from multiple doc, but all this
> docs need to cancel it for the prefetch to be cancelled?  I think we need
> that, e.g. for fonts.
Could each request increase some counter and each cancelling decrease it and only
counter == 0 would actually cancel it?


> 
> 3) Our prefetch and preconnect logic does not have a notion of probability,
> we could add that (maybe opening separate bug for that)
Right. Also the spec if _very_ vague what probability means.
In case of prefetch it should do something with "next navigation context", and nothing defines
what is "next navigation context".
There is a spec bug https://github.com/w3c/resource-hints/issues/48


> 4) We prefetch only when load of all docs is finished (nothing is loading)
> and if load of a doc starts we cancel and delete all prefeteches currently
> in progress or queued. A scenario: you click on a link and a script adds a
> <link> for prefetching some resource that that the new page will need. This
> scenario will not work with current prefetch service  because the prefetch
> will  be cancelled as soon as the new load starts. This will need more
> rework to decide which prefatch to keep and which to cancel.
Do we need to cancel anything? Could we just use lowest priority request for prefetch (even during page load?) and only cancel those prefetch requests which have been loaded using non-prefetch mechanisms?
I'm not too familiar the necko side of this, but at least there used to be the notion of priority on requests.
Comment on attachment 8710064 [details] [diff] [review]
bug_580313_v1.patch

I will do an update here and ask for feedback.
Attachment #8710064 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] (expect slower than usual review time for couple of days) from comment #27)
> (In reply to Dragana Damjanovic [:dragana] from comment #25)
> > The first version is finished, but there is a couple of thing that we need
> > to decide upon:
> > 1) This patch does not do prerander (<link rel="preload"
> > loadpolicy="next"...), instead it does just prefetch. I will need some help
> > with that (I am not really familiar with dom but willing to learn it). And
> > we need to decide how to load-balance that (maybe there is something for
> > that).
> >
> Not sure what this is about. prerander? you mean prerender but that is all
> different.
> and what is loadpolicy with preload? I don't see that in 
> https://w3c.github.io/resource-hints/

We had `loadpolicy` in early draft of the spec but it was removed in favor of rel=prefetch. With that in mind, I think this is a noop?

> > 4) We prefetch only when load of all docs is finished (nothing is loading)
> > and if load of a doc starts we cancel and delete all prefeteches currently
> > in progress or queued. A scenario: you click on a link and a script adds a
> > <link> for prefetching some resource that that the new page will need. This
> > scenario will not work with current prefetch service  because the prefetch
> > will  be cancelled as soon as the new load starts. This will need more
> > rework to decide which prefatch to keep and which to cancel.
> Do we need to cancel anything? Could we just use lowest priority request for
> prefetch (even during page load?) and only cancel those prefetch requests
> which have been loaded using non-prefetch mechanisms?
> I'm not too familiar the necko side of this, but at least there used to be
> the notion of priority on requests.

Note that the "reactive prefetch" example you described above is an important use case - e.g. [1]. Similarly, killing prefetch requests on navigation limits its usefulness because such requests must complete before parent page is unloaded. That said, these should probably live in a separate bug, since this one is specifically for starting prefetch on DOM mutations?

[1] https://plus.google.com/+IlyaGrigorik/posts/ahSpGgohSDo
Attached patch bug_580313_v1.patch (obsolete) — Splinter Review
Attachment #8710064 - Attachment is obsolete: true
Attachment #8711037 - Flags: feedback?(bugs)
Comment on attachment 8711037 [details] [diff] [review]
bug_580313_v1.patch

Someone else needs to look at the necko part


>+Link::TryPrefetchHrefPreconnectDNSPrefetch()
This is a long name, which is fine, but to my non-native-English speaker ears not too clear.
I guess it is mostly that 'Href'. Perhaps drop that?


>+{
>+  MOZ_ASSERT(mElement->IsInComposedDoc());
>+  if (!ElementHasHref()) {
>+    return;
>+  }
>+
>+  nsAutoString rel;
>+  if (!mElement->GetAttr(kNameSpaceID_None, nsGkAtoms::rel, rel)) {
>+    return;
>+  }
>+
>+  uint32_t linkTypes = nsStyleLinkElement::ParseLinkTypes(rel,
>+                         mElement->NodePrincipal());
>+
>+  if ((linkTypes & nsStyleLinkElement::ePREFETCH) ||
>+      (linkTypes & nsStyleLinkElement::eNEXT)){
>+    bool prefetch = true;
>+    //
>+    // SECURITY CHECK: disable prefetching from mailnews!
>+    //
>+    // walk up the docshell tree to see if any containing
>+    // docshell are of type MAIL.
>+    //
>+    nsCOMPtr<nsIDocShell> docshell = mElement->OwnerDoc()->GetDocShell();
>+    if (docshell) {
>+      nsCOMPtr<nsIDocShellTreeItem> parentItem;
>+      do {
>+        uint32_t appType = 0;
>+        nsresult rv = docshell->GetAppType(&appType);
>+        if (NS_FAILED(rv) || appType == nsIDocShell::APP_TYPE_MAIL) {
>+          prefetch = false;
>+          break; // do not prefetch from mailnews
>+        }
>+        docshell->GetParent(getter_AddRefs(parentItem));
>+        if (parentItem) {
>+          docshell = do_QueryInterface(parentItem);
>+          if (!docshell) {
>+            NS_ERROR("cannot get a docshell from a treeItem!");
>+            prefetch = false;
>+            break;
>+          }
>+        }
>+      } while (parentItem);

actually, I think we should move this TYPE_MAIL check our from 
">+  if ((linkTypes & nsStyleLinkElement::ePREFETCH) ||
>+      (linkTypes & nsStyleLinkElement::eNEXT)){"
so that all the types get the same handling. Also ContentSink should do that consistently 



>+    }
>+    if (prefetch) {
nit, a new line before 'if (prefetch) {' would make the code a bit easier to read 


>+      nsCOMPtr<nsIPrefetchService> prefetchService(do_GetService(NS_PREFETCHSERVICE_CONTRACTID));
>+      if (prefetchService) {
>+        nsCOMPtr<nsIURI> uri(GetURI());
>+        if (uri) {
>+          nsCOMPtr<nsIDOMNode> domNode = do_QueryInterface(mElement->OwnerDoc());
mElement->OwnerDoc()->AsDOMNode() is a bit easier and faster.
But why do we need the document here? why not mElement itself? mElement->AsDOMNode() ?
Attachment #8711037 - Flags: feedback?(bugs) → feedback+
Attached patch bug_580313_v2.patch (obsolete) — Splinter Review
The necko is not change here, but /uriloader/prefetch/nsPrefetchService is. I have seen that bz had reviewed some patches there so I will ask him. 

bz if you are not the right person do you know who is?
Attachment #8711037 - Attachment is obsolete: true
Attachment #8712084 - Flags: review?(bzbarsky)
Attachment #8712084 - Flags: review?(bugs)
Oh, indeed. Somehow I just assumed prefetch was in necko. sorry about that.
Comment on attachment 8712084 [details] [diff] [review]
bug_580313_v2.patch

And I guess either I or bz should review this all. No need for both.
Attachment #8712084 - Flags: review?(bzbarsky)
Attached patch bug_580313_v2.patch (obsolete) — Splinter Review
I have only change test that was failing.

Because I do not know dom that well, I am writing what is the difference:
the previous version was sending 2 preconnects for each link and now because of the check added (moved) in nsContentSink::ProcessLink mDocShell is nullptr so preconnect is not perform here.
Attachment #8712084 - Attachment is obsolete: true
Attachment #8712084 - Flags: review?(bugs)
Attachment #8713590 - Flags: review?(bugs)
Comment on attachment 8713590 [details] [diff] [review]
bug_580313_v2.patch

Sorry about delay!


>+  if ((linkTypes & nsStyleLinkElement::ePREFETCH) ||
>+      (linkTypes & nsStyleLinkElement::eNEXT)){
>+    nsCOMPtr<nsIPrefetchService> prefetchService(do_GetService(NS_PREFETCHSERVICE_CONTRACTID));
>+    if (prefetchService) {
>+      nsCOMPtr<nsIURI> uri(GetURI());
>+      if (uri) {
>+        nsCOMPtr<nsIDOMNode> domNode = mElement->OwnerDoc()->AsDOMNode();
>+        prefetchService->PrefetchURI(uri,
>+                                     mElement->OwnerDoc()->GetDocumentURI(),
>+                                     domNode, true);
So why you pass element's owner document as the source?
If there are several links to the same uri, those all will be handled as one source and cancelling any of them will cancel all.



>+  //
>+  // SECURITY CHECK: disable prefetching from mailnews!
>+  //
>+  // walk up the docshell tree to see if any containing
>+  // docshell are of type MAIL.
>+  //
>+
>+  if (!mDocShell) {
>+    return NS_OK;
>+  }
>+
>+  nsCOMPtr<nsIDocShell> docshell = mDocShell;
>+  nsCOMPtr<nsIDocShellTreeItem> parentItem;
>+
>+  do {
>+    uint32_t appType = 0;
>+    nsresult rv = docshell->GetAppType(&appType);
>+    if (NS_FAILED(rv) || appType == nsIDocShell::APP_TYPE_MAIL) {
>+      return NS_OK; // do not prefetch, preconnect from mailnews
>+    }
>+
>+    docshell->GetParent(getter_AddRefs(parentItem));
>+    if (parentItem) {
>+      docshell = do_QueryInterface(parentItem);
>+      if (!docshell) {
>+        NS_ERROR("cannot get a docshell from a treeItem!");
>+        return NS_OK;
>+      }
>+    }
>+  } while (parentItem);
>+
>+  // OK, we passed the security check...
Could you please have this code only in one place. Add some helper method.
Maybe to nsContentUtils, it could take nsIDocShell as param


> NS_IMETHODIMP
>+nsPrefetchService::CancelPrefetchURI(nsIURI *aURI,
>+                                     nsIDOMNode *aSource)
Nit, in new code * should go with the type.
(though, this is kind of old code using 4 spaces for indentation and what not.)


>+    //
>+    // look into the prefetch queue
>+    //
>+    nsPrefetchNode *node = mQueueHead;
>+    nsPrefetchNode *prevNode = nullptr;
>+    for (; node; node = node->mNext) {
>+        bool equals;
>+        if (NS_SUCCEEDED(node->mURI->Equals(aURI, &equals)) && equals) {
>+            nsCOMPtr<nsIWeakReference> source = do_GetWeakReference(aSource);
>+            if (node->mSources.IndexOf(source) !=
>+                node->mSources.NoIndex) {
>+                node->mSources.RemoveElement(source);
>+                if (node->mSources.IsEmpty()) {
So do we guarantee that all the objects are removed from mSources at some point.
Remember, it just contains weakrefs. So the weakref pointer itself might be alive, but 
do_QueryReferent on it might return null. If we do guarantee that, at least add a #ifdef DEBUG check that
do_QueryReferent on each source object gives a non-null value in case the array is not empty.

>+                    if (!prevNode) {
>+                        mQueueHead = mQueueHead->mNext;
>+                        if (!mQueueHead) {
>+                            mQueueTail = nullptr;
>+                        }
>+                    } else {
>+                        prevNode->mNext = node->mNext;
>+                        if (!prevNode->mNext) {
>+                            mQueueTail = prevNode;
>+                        }
>+                    }
                      So you leak node here, right? It is manually refcounted (yes, totally crazy old code).
                      The leak should show up in debug builds if you have XPCOM_MEM_LEAK_LOG=1 env variable set.

                      And don't you need to update prevNode on each iteration. Not only inside the 'if (url-check)'.

                      Do we have any tests for checking this cancelling behavior?


>+                }
>+            }
>+            return NS_ERROR_FAILURE;
              Why you return NS_ERROR_FAILURE here?
Attachment #8713590 - Flags: review?(bugs) → review-
Attached patch part0_bug_580313_v1.patch (obsolete) — Splinter Review
I have change nsPrefetchService to use deque. :)
Attachment #8713590 - Attachment is obsolete: true
Attachment #8719877 - Flags: review?(bugs)
Attached patch part1_bug_580313_v1.patch (obsolete) — Splinter Review
Attachment #8719878 - Flags: review?(bugs)
Comment on attachment 8719877 [details] [diff] [review]
part0_bug_580313_v1.patch


>+    RefPtr<nsPrefetchNode> node = new nsPrefetchNode(this, aURI, aReferrerURI,
>+                                                       aSource);
nit, align aSource


>+    std::deque<RefPtr<nsPrefetchNode>> mQueue;
>+    nsTArray<RefPtr<nsPrefetchNode>>     mCurrentNodes;
>+    int32_t                              mMaxParallelism;
>+    int32_t                              mStopCount;
Nit, something odd here with alignment.
Attachment #8719877 - Flags: review?(bugs) → review+
Comment on attachment 8719878 [details] [diff] [review]
part1_bug_580313_v1.patch

>@@ -911,16 +911,17 @@ public:
>    */
>   static uint32_t ParseSandboxAttributeToFlags(const nsAttrValue* sandboxAttr);
> 
>   /**
>    * Helper function that generates a UUID.
>    */
>   static nsresult GenerateUUIDInPlace(nsID& aUUID);
> 
>+  static bool DoPrefetch(nsIDocShell* aDocShell);
This sounds like calling the method would prefetch something.
Perhaps call this "PrefetchEnabled"



>+var docbody = '<html xmlns="http://www.w3.org/1999/xhtml"><head></head><body>' +
>+              '<link id="node1"/><link id="node2"/>' +
>+              '</body></html>';
>+
>+var node1;
>+var node2;
>+
>+function run_test() {
>+  prefs.setBoolPref("network.prefetch-next", true);
>+
>+  parser.init();
>+  doc = parser.parseFromString(docbody, "text/html");
So using this kind of test is currently fine, but I assume it is a bug in spec that it doesn't 
require the document to have browsing context (document.defaultView != null)
Mochitest would be better here. But I guess that can be fixed if/when the spec and our implementation are updated.


But I don't see any test here to actually check what happens when a node which has the prefetch attribute value etc. is removed from document.
Or what happens when the attribute value is changed or so.
Why explicitly use nsIPrefetchService to do prefetch or cancel it. You should be able to test using elements, by adding and removing them to document
and changing attributes, and then perhaps use prefetch.hasMoreElements() to check that the dom mutations caused the right prefetch behavior.



> nsPrefetchNode::OpenChannel()
> {
>-    nsCOMPtr<nsINode> source = do_QueryReferent(mSource);
>+    if (mSources.IsEmpty()) {
>+        // Don't attempt to prefetch if we don't have a source node
>+        // (which should never happen).
>+        return NS_ERROR_FAILURE;
>+    }
>+    nsCOMPtr<nsINode> source = do_QueryReferent(mSources.ElementAt(0));
>     if (!source) {
If we get null when calling do_QueryReferent, we should remove first element from mSources and try the next one in that array and so on


I think I'd like to see a new patch.
Attachment #8719878 - Flags: review?(bugs) → review-
Attachment #8719877 - Attachment is obsolete: true
Attachment #8722090 - Flags: review+
Attached patch part1_bug_580313_v2.patch (obsolete) — Splinter Review
Attachment #8719878 - Attachment is obsolete: true
Comment on attachment 8722124 [details] [diff] [review]
part1_bug_580313_v2.patch

I added one more test. I am keeping them both, because they are a bit different.
Attachment #8722124 - Flags: review?(bugs)
Comment on attachment 8722124 [details] [diff] [review]
part1_bug_580313_v2.patch



>+
>+  uint32_t linkTypes = nsStyleLinkElement::ParseLinkTypes(rel,
>+                         mElement->NodePrincipal());
>+
>+  if ((linkTypes & nsStyleLinkElement::ePREFETCH) ||
>+      (linkTypes & nsStyleLinkElement::eNEXT)){
>+    nsCOMPtr<nsIPrefetchService> prefetchService(do_GetService(NS_PREFETCHSERVICE_CONTRACTID));
>+    if (prefetchService) {
>+      nsCOMPtr<nsIURI> uri(GetURI());
>+      if (uri) {
>+        nsCOMPtr<nsIDOMNode> domNode = GetAsDOMNode(mElement);
>+        prefetchService->PrefetchURI(uri,
>+                                     mElement->OwnerDoc()->GetDocumentURI(),
>+                                     domNode, true);
>+        return;
Why this early return? rel attribute contains a list of tokens so could in theory be used to define several resource hints.

>+  if (linkTypes & nsStyleLinkElement::ePRECONNECT) {
>+    nsCOMPtr<nsIURI> uri(GetURI());
>+    if (uri && mElement->OwnerDoc()) {
>+      mElement->OwnerDoc()->MaybePreconnect(uri,
>+        mElement->AttrValueToCORSMode(mElement->GetParsedAttr(nsGkAtoms::crossorigin)));
>+      return;
ditto


>+bool
>+nsContentUtils::PrefetchEnabled(nsIDocShell* aDocShell)
>+{
>+  //
>+  // SECURITY CHECK: disable prefetching from mailnews!
>+  //
>+  // walk up the docshell tree to see if any containing
>+  // docshell are of type MAIL.
>+  //
>+
>+  if (!aDocShell) {
>+    return false;
>+  }
Ok, this isn't per spec, since the spec doesn't care about whether we're dealing with data documents or document with browsing context.
But there is a spec bug about this, and I think this behavior makes sense.


>     if (!source) {
>         // Don't attempt to prefetch if we don't have a source node
>         // (which should never happen).
>+
>         return NS_ERROR_FAILURE;
some odd extra new line here before 'return'.
Attachment #8722124 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #47)
> Comment on attachment 8722124 [details] [diff] [review]
> part1_bug_580313_v2.patch

> 
> >+bool
> >+nsContentUtils::PrefetchEnabled(nsIDocShell* aDocShell)
> >+{
> >+  //
> >+  // SECURITY CHECK: disable prefetching from mailnews!
> >+  //
> >+  // walk up the docshell tree to see if any containing
> >+  // docshell are of type MAIL.
> >+  //
> >+
> >+  if (!aDocShell) {
> >+    return false;
> >+  }
> Ok, this isn't per spec, since the spec doesn't care about whether we're
> dealing with data documents or document with browsing context.
> But there is a spec bug about this, and I think this behavior makes sense.
> 
> 

I had another look at this, so that I do not make a regression.
And it does. 

I do not know dom that well so i need your help here. This check will prevent link response header to be preconnected or prefetch because they do not have a docShell. What is the right think to do here?
Thanks a lot!
Flags: needinfo?(bugs)
So IMO it makes no sense for <link> elements in documents which don't have browsing context to trigger any prefetch or other resource hints.
The spec bug is https://github.com/w3c/resource-hints/issues/49

If possible, could you check what Chrome does here.
Hopefully it is against the current spec :) (and we could just do the sane thing, and not trigger resource hints without browsing context.)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #49)
> So IMO it makes no sense for <link> elements in documents which don't have
> browsing context to trigger any prefetch or other resource hints.
> The spec bug is https://github.com/w3c/resource-hints/issues/49
> 
> If possible, could you check what Chrome does here.
> Hopefully it is against the current spec :) (and we could just do the sane
> thing, and not trigger resource hints without browsing context.)


Chrome:
document.implementation.createHTMLDocument().body.innerHTML = "<link rel=" works if rel is preconnect and it is ignored it if it prefetch. 

fetching something xhr with a header Link behaves the same.

In current Firefox document.implementation.createHTMLDocument().body.innerHTML = "<link rel=" behaves the same as chrome and xhr with a header Link are ignored prefetch and preconnect.

with this patch any prefetch, preconnect or dns-prefetch will be ignored if a documment does not have browser context.
Could you file a crbug.com issue and send it my way? Thanks! :)
dragana, I think we should get the behavior your patch gives, especially now that blink folks are ready
to fix their implementation and I haven't seen any comments against that behavior in the spec bug either.
I ave only fix a test in this update.
Attachment #8722124 - Attachment is obsolete: true
Attachment #8724006 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ef1fb285a58d
https://hg.mozilla.org/mozilla-central/rev/c4d101e34585
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Depends on: 1279440
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.