Closed Bug 1134648 Opened 5 years ago Closed 4 years ago

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

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: yves.vangoethem, Assigned: smaug)

References

()

Details

Attachments

(2 files, 1 obsolete file)

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
Component: Untriaged → Networking: DNS
Product: Firefox → Core
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(jduell.mcbugs)
I can take a look.
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.
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)
I have adde similar thing that HTMPAnchorElement does.
I am not really familiar with this code.
Attachment #8567019 - Flags: feedback?(khuey)
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.
@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
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?
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)
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?
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-
*bump*

Anything we can do to unblock this and related dynamic-hints use cases?
Flags: needinfo?(dd.mozilla)
Assignee: nobody → bugs
Attached patch v2 (obsolete) — Splinter Review
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)
Attached patch v3Splinter Review
Attachment #8692269 - Attachment is obsolete: true
Attachment #8692269 - Flags: feedback?(sworkman)
Attachment #8692561 - Flags: feedback?(sworkman)
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.
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+
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+
https://hg.mozilla.org/mozilla-central/rev/3254063d0c0a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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.