dns-prefetch is not happening when link is appended to the document after window.onload

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: yves.vangoethem, Assigned: smaug)

Tracking

unspecified
mozilla45
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox45 fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/40.0.2214.111 Safari/537.36

Steps to reproduce:

This test: http://jsbin.com/domuniqihe/2/edit
Appends a <link rel="dns-prefetch" href="…"> to document.head after window.onload


Actual results:

The DNS was not prefetched, and the DNS lookup happens when the image is appended to the document after a 3 second timeout.
A waterfall can be seen here: http://www.webpagetest.org/result/150219_K3_6f4b266d4799d6afa98c95e8acbdd009/1/details/


Expected results:

The browser should pick up the added link tag to the document and prefetch the host like it does for already present link element while parsing the document, 3 seconds before the image loads from http://fortunes.inertie.org/custom/img/h1.png
A waterfall like the one Chrome displays: http://www.webpagetest.org/result/150219_86_23f61a43ed4f7d9ec5d99fe88472e50e/1/details

Updated

4 years ago
Component: Untriaged → Networking: DNS
Product: Firefox → Core

Updated

4 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jduell.mcbugs)
I can take a look.

Updated

4 years ago
Assignee: nobody → dd.mozilla
Looking at this, it seams that PrefetchDNS is never called. I will ask over there.
Assignee: dd.mozilla → nobody
Component: Networking: DNS → DOM: Core & HTML
PrefetchDNS is called by the parser.  So it clearly won't be called for links that are added via a non-parser mechanism.

Comment 4

4 years ago
Ah, I had assumed that this code would take care of it:

  https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLAnchorElement.cpp?from=HTMLAnchorElement.cpp#157

but apparently not.  Hopefully someone from the DOM side of things can look into the plumbing we're missing here.  Andrew, do you know who could take this?
Flags: needinfo?(jduell.mcbugs) → needinfo?(overholt)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #4)
> Ah, I had assumed that this code would take care of it:
> 
>  
> https://dxr.mozilla.org/mozilla-central/source/dom/html/HTMLAnchorElement.
> cpp?from=HTMLAnchorElement.cpp#157
> 
> but apparently not.  Hopefully someone from the DOM side of things can look
> into the plumbing we're missing here.  Andrew, do you know who could take
> this?

Well sure, that would take care of it for a <a> tag.  But this is a <link>, which has no corresponding code in BindToTree, AFAICT.
Flags: needinfo?(overholt)
Created attachment 8567019 [details] [diff] [review]
bug_1134648_v1.patch

I have adde similar thing that HTMPAnchorElement does.
I am not really familiar with this code.
Attachment #8567019 - Flags: feedback?(khuey)
(Assignee)

Comment 7

4 years ago
This sounds similar to bug 580313, Do we actually have a reasonable spec for this stuff? (I doubt)
But this should be fixed anyhow, and relevant spec bugs filed.

Comment 8

4 years ago
@Olli: Yes, it's the exact same issue in disguise.. FF is not processing dynamically inserted <link> relations -- applies to prefetch, dns-prefetch, etc. 

Re, spec: There is a mention for this in [1], but I'm sure it can be fleshed out further and be improved. If you have any suggestions, let me know.

[1] https://w3c.github.io/preload/#dynamic-scheduling
(Assignee)

Comment 9

4 years ago
That vaguely defines something about preload, not dns-prefetch.

And even for preload it doesn't define 'inserted where'. It just says
"the application may dynamically insert and remove additional preload relations via JavaScript. "
That doesn't really mean anything. Is creating a new link element with right attributes an insertion?
Is inserting such link element to random DOM subtree (which isn't in Document) such insertion?
or what?

Comment 10

4 years ago
Olli: opened a bug to track this: https://github.com/w3c/resource-hints/issues/25. If you have any suggestions for appropriate spec language + behaviors, that'd be much appreciated :)
Comment on attachment 8567019 [details] [diff] [review]
bug_1134648_v1.patch

I don't think I'm the right person to review this.  Maybe smaug?
Attachment #8567019 - Flags: feedback?(khuey) → feedback?(bugs)
(Reporter)

Comment 12

4 years ago
Olli: I wrote an additional test to look at why Chrome is doing:
 
http://jsbin.com/kuluqipuba/2
http://www.webpagetest.org/result/150223_MP_302a68179e1c39b71532694b3e415a75/1/details/

It looks like the DNS isn't prefetched when the LINK element isn't appended to the Document.
I'm wondering if Firefox should actually introduce this, but perhaps it would be more productive to move the specs to another discussion, and aim for feature equality with an existing browser in this bug?
(Assignee)

Comment 13

4 years ago
Comment on attachment 8567019 [details] [diff] [review]
bug_1134648_v1.patch

Why do we need the flags? We have that information already as attribute value.
Attachment #8567019 - Flags: feedback?(bugs) → feedback-

Comment 14

3 years ago
*bump*

Anything we can do to unblock this and related dynamic-hints use cases?
(Assignee)

Updated

3 years ago
Flags: needinfo?(dd.mozilla)
(Assignee)

Updated

3 years ago
Assignee: nobody → bugs
(Assignee)

Comment 15

3 years ago
Created attachment 8692269 [details] [diff] [review]
v2

Apparently <a> uses similar flags after all, which is a bit odd, but decided to reused that setup, even if it has some
unexpected behavior like PREFETCH_REQUESTED isn't necessarily ever unset etc.

But need to take care of changes to attributes too and actually check there is
rel="dns-prefetch" etc.


But I don't know how to test this automatically. Do we have any way for that?
Bug 622232 didn't add tests. sworkman do you know?
(I used wireshark locally, and need to still go through all the different cases.)


Filed spec bugs https://github.com/w3c/resource-hints/issues/48
and https://github.com/w3c/resource-hints/issues/49
(looks like we require prefetch element to be in a document which has browsing context, but document doesn't need to
be the active one. See nsHTMLDNSPrefetch::IsAllowed)
Attachment #8692269 - Flags: feedback?(sworkman)
(Assignee)

Comment 16

3 years ago
Created attachment 8692561 [details] [diff] [review]
v3
Attachment #8692269 - Attachment is obsolete: true
Attachment #8692269 - Flags: feedback?(sworkman)
Attachment #8692561 - Flags: feedback?(sworkman)
(Assignee)

Comment 17

3 years ago
Comment on attachment 8692561 [details] [diff] [review]
v3

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9829fe31bbfa


parser/html/nsHtml5DocumentBuilder.cpp/dom/xml/nsXMLContentSink.cpp
shouldn't need to deal with dns-prefetch. In fact, because of them we trigger
dns-prefetch even for elements which aren't in document when
innerHTML = "<link ...>" is used on an element which isn't in document.

But still don't know whether we have any way to test this automatically.
(Assignee)

Updated

3 years ago
Attachment #8692561 - Flags: review?(bzbarsky)
Comment on attachment 8692561 [details] [diff] [review]
v3

Review of attachment 8692561 [details] [diff] [review]:
-----------------------------------------------------------------

feedback+ Looks somewhat sane to me, but you should def trust bz more than me. It's been a while since I worked on the DNS prefetch code, and I wasn't super familiar with the calling code at the time, never mind now ;)

(In reply to Olli Pettay [:smaug] from comment #15)
> But I don't know how to test this automatically. Do we have any way for that?
> Bug 622232 didn't add tests. sworkman do you know?
> (I used wireshark locally, and need to still go through all the different
> cases.)

No, we didn't have a way to do automated testing for this back then, and I don't think we have anything now either. But Dragana or Jduell will have more up-to-date knowledge.
Attachment #8692561 - Flags: feedback?(sworkman) → feedback+
(Assignee)

Comment 19

3 years ago
I used wireshark for testing, but anything automatic? Dragana, do you happen to know?
Or perhaps you have some manual tests which you could run?
Comment on attachment 8692561 [details] [diff] [review]
v3

r=me
Attachment #8692561 - Flags: review?(bzbarsky) → review+

Comment 22

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3254063d0c0a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45

Comment 23

3 years ago
Hooray! Curious, do we now also need to mirror this for prefetch as well?

https://bugzilla.mozilla.org/show_bug.cgi?id=580313
(In reply to Olli Pettay [:smaug] from comment #19)
> I used wireshark for testing, but anything automatic? Dragana, do you happen
> to know?
> Or perhaps you have some manual tests which you could run?

I do not know any automatic tests for this. I think I was using logging to be test the first patch, but nothing automatic.
Flags: needinfo?(dd.mozilla)
You need to log in before you can comment on or make changes to this bug.