Link prefetching needs to open its channel with the correct loadgroup for the document it belongs to

RESOLVED FIXED in Firefox 18

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Away for a while, Assigned: Away for a while)

Tracking

Trunk
mozilla18
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox18+ fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Boris, can nsPrefetchNode::OpenChannel use the load group for mSource->OwnerDoc()?
(Assignee)

Updated

5 years ago
tracking-firefox18: --- → +
I'm 99.9% sure mSource is always null in practice.

If that were fixed, then yes, it could.
(Assignee)

Comment 2

5 years ago
(In reply to comment #1)
> I'm 99.9% sure mSource is always null in practice.
> 
> If that were fixed, then yes, it could.

I'm new to this code, but as far as I can tell, prefetching is triggered from here <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#834>, and the only call site which passes in null is: <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#691>, which seems to be triggered from here: <http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsContentSink.cpp#353>.  Does the link element exist when that code is called?  If not, I don't see how this can be (easily) fixed...
(Assignee)

Comment 3

5 years ago
Hmm, we could also modify the prefetch service to accept a source document, and just grab the load context from the document directly.  Would that be a better solution?
Oh, we no longer go through ProcessLink for <link> elements, apparently!

In that case, the only case mSource will be null in is an HTTP <link> header.  In which case, we should arguably pass in the document node as the source node.  And then use the loadgroup of mSource->OwnerDoc().
(Assignee)

Comment 5

5 years ago
Created attachment 659038 [details] [diff] [review]
Part 1: When prefetching a document using a Link header, use the document element as the source node
Attachment #659038 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

5 years ago
Created attachment 659047 [details] [diff] [review]
Part 2: Use the load group of the document that the source node belongs to when prefetching links
Attachment #659047 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #659047 - Flags: review? → review?(bzbarsky)
Comment on attachment 659038 [details] [diff] [review]
Part 1: When prefetching a document using a Link header, use the document element as the source node

I'd prefer it if we changed PrefetchHref to take an nsINode and passed in mDocument here, perhaps.  But this is OK too.
Attachment #659038 - Flags: review?(bzbarsky) → review+
Comment on attachment 659047 [details] [diff] [review]
Part 2: Use the load group of the document that the source node belongs to when prefetching links

I think if !source we should just not prefetch.  Thoughts?
(Assignee)

Comment 9

5 years ago
(In reply to Boris Zbarsky (:bz) from comment #8)
> Comment on attachment 659047 [details] [diff] [review]
> Part 2: Use the load group of the document that the source node belongs to
> when prefetching links
> 
> I think if !source we should just not prefetch.  Thoughts?

Hmm, that makes sense.  I'll do that in a new version of this patch.
(Assignee)

Comment 10

5 years ago
Created attachment 659294 [details] [diff] [review]
Part 1: When prefetching a document using a Link header, use the document element as the source node
Attachment #659038 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created attachment 659297 [details] [diff] [review]
Part 2: Use the load group of the document that the source node belongs to when prefetching links
Attachment #659047 - Attachment is obsolete: true
Attachment #659047 - Flags: review?(bzbarsky)
Attachment #659297 - Flags: review?(bzbarsky)
Comment on attachment 659297 [details] [diff] [review]
Part 2: Use the load group of the document that the source node belongs to when prefetching links

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

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1e9a9400f07
https://hg.mozilla.org/integration/mozilla-inbound/rev/672a11af05c3
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/e1e9a9400f07
https://hg.mozilla.org/mozilla-central/rev/672a11af05c3
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox18: --- → fixed
You need to log in before you can comment on or make changes to this bug.