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

Status: RESOLVED FIXED

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

Description

[Yves Van Goethem]

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

Comment 1

[Dragana Damjanovic [:dragana]]

I can take a look.

Comment 2

[Dragana Damjanovic [:dragana]]

Looking at this, it seams that PrefetchDNS is never called. I will ask over there.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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

[Jason Duell]

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?

Comment 5

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(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.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: bug_1134648_v1.patch

I have adde similar thing that HTMPAnchorElement does.
I am not really familiar with this code.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

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

[Ilya Grigorik]

@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

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

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

[Ilya Grigorik]

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 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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?

Comment 12

[Yves Van Goethem]

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 13

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 14

[Ilya Grigorik]

*bump*

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

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: 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)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v3

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

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.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

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 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8692561 [details] [diff] [review]
v3

r=me

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3254063d0c0a

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3254063d0c0a

Comment 23

[Ilya Grigorik]

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

https://bugzilla.mozilla.org/show_bug.cgi?id=580313

Comment 24

[Dragana Damjanovic [:dragana]]

(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.