Closed Bug 1268962 Opened 8 years ago Closed 8 years ago

Fire load/error events on <link rel="prefetch">

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
platform-rel --- ?
firefox49 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: bzbarsky, Assigned: freesamael, NeedInfo)

References

Details

(Whiteboard: btpp-active [platform-rel-Amazon])

Attachments

(1 file, 6 obsolete files)

Add load / error event to prefetch link. r=bz
43.13 KB, patch
freesamael
: review+
Details | Diff | Splinter Review 
Testcase:

  data:text/html,<link rel="prefetch" href="http://web.mit.edu" onload="alert(1)" onerror="alert(2)">

this alerts 1 in Chrome.  On the other hand,

  data:text/html,<link rel="prefetch" href="http://test:test" onload="alert(1)" onerror="alert(2)">

does not alert anything.  And we should keep in mind the discussion in https://github.com/whatwg/html/issues/1142

When this is fixed, we will probably want to remove the ini files the following command finds:

  find testing/web-platform/meta/mixed-content/ -name "*.ini" | grep link-prefetch-tag

Nick, do you have time to look into this?
Flags: needinfo?(hurley)
Eric, what are your constraints here in terms of wanting to distinguish between success and failure of loads for cross-origin loads?  Do you need it at all?  Would you be OK with some mitigations for the issues described in https://github.com/whatwg/html/issues/1142 like pretending a cross-origin load is always successful?  With pretending that a cross-origin 404 is actually successful?
Flags: needinfo?(ericsc)
Blocks: 1172205
See also bug 1172205 comment 48 through bug 1172205 comment 54 for the discussion about this.

Filed spec issue at https://github.com/whatwg/html/issues/1148
Whiteboard: btpp-followup-2016-05-06
Oh, there's a spec kinda.  OK.  I don't see how it can be made less vague offhand, but we should at least align across UAs on the cross-origin situation.
well, stuff like... does removing link element from DOM cause an error event? 

And the language is a bit odd to my non-native-English-speaker ears "whether a resource hint is executed ... user agent SHOULD fire the appropriate load and error events when possible, to allow the application to track which hints were executed". Prefetch is not something you "execute", especially when elsewhere in the spec it says about prefetch that "...MUST NOT automatically execute"

But yes, we can implement something here while fixing the spec issues.

Ilya, could you perhaps help with sorting out the spec issues here.
Flags: needinfo?(igrigorik)
>Eric, what are your constraints here in terms of wanting to distinguish between success and failure of loads for cross-origin loads?  Do you need it at all?  

Yup - we download assets from CDN's on different domains. 

We want to be able to detect success and failure so we can manage how many requests we make, and keep track of what we've successfully loaded and not.
OK.  Do you need to specifically distinguish between 404 and 200 responses?  That is, are you OK with treating 404 as "success" for purposes of load/error events here?

If not, we will probably need some sort of CORS setup here to not neuter the cross-domain success state.
Richard, this too is blocked on <https://github.com/whatwg/html/issues/1142>....
Flags: needinfo?(rlb)
(In reply to Boris Zbarsky [:bz] from comment #0)
> Nick, do you have time to look into this?

Ugh, got lost in the shuffle of my life. Sorry. I am not currently available to look into this - too many fires with predictive prefetch that need putting out first.
Flags: needinfo?(hurley)
Andrew, can we find someone to take a look at this, both in terms of implementation and the security issues?  This is the one remaining known blocker to getting Amazon to stop doing the really slow/annoying stuff described in bug 1172205...
Flags: needinfo?(overholt)
I'm exploring a few options.
Flags: needinfo?(overholt)
Hi Samael, per our chat, please help with this. Let's see if we could make it happen in Firefox 50, by 2016-08-01. Thank you!
Assignee: nobody → sawang
Whiteboard: btpp-followup-2016-05-06 → btpp-active
Flags: needinfo?(rlb)
Richard, we really do need a security evaluation here.  What do we need to do here to avoid cross-site information leaks while still providing a useful feature?
Flags: needinfo?(rlb)
It seems gecko didn't implement prefetch link as an optionally
blockable item yet. It blocks the link in mixed content case.
Until we implement the feature, expect fail for these test cases.
Comment on attachment 8770468 [details] [diff] [review]
Part 1: Add load / error event to prefetch link

Hi BZ,

It seems we haven't got conclusion about how cross origin 404 should be handled, but in the mean time could you give me some feedback on the basic implementation?
Attachment #8770468 - Flags: feedback?(bzbarsky)
Attachment #8770469 - Flags: feedback?(bzbarsky)
Attachment #8770470 - Flags: feedback?(bzbarsky)
Comment on attachment 8770468 [details] [diff] [review]
Part 1: Add load / error event to prefetch link

This generally seems reasonable; the edge cases might need some careful checking in terms of whether to fire load or error.

In DispatchEvent, domNode might be null.  You need to just skip it if so.  Also, why an untrusted event?  Seems to me like this should be trusted.
Attachment #8770468 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8770469 [details] [diff] [review]
Part 2: Enable prefetch web-platform-tests

r=me but please fold with the actual patch when checking in, right?
Attachment #8770469 - Flags: feedback?(bzbarsky) → review+
Comment on attachment 8770470 [details] [diff] [review]
Part 3: Expect fail for no-opt-in test cases

Again, r=me but please fold with the actual behavior change.
Attachment #8770470 - Flags: feedback?(bzbarsky) → review+
OK, :bz and I had some discussion offline, and I think I understand the problem better now.  Let me try to summarize.

# Summary

With the current DOM API, a web developer can use elements that (1) send requests cross-origin and (2) fire onload/onerror (like <img> and <link rel="stylesheet">) to probe for the existence of resources across origins.  However, those channels are restricted, e.g., by content type.

Enabling onload / onerror for <link rel="prefetch"> would provide a general probing mechanism.  Put any URL you want in there, and the browser will let you know if it returns an error status or not.

# Analysis

To be honest, I'm not sure how big a deal this is.  

If we just fire the events, with no other changes, then I'm concerned, but I'm not sure how much.  Clearly there is some additional surface that this would expose, but it's not clear whether it makes that much difference in practice.  I'll send a ping to the WebAppSec mailing list, where there are a few security sensitive web developers.

If were to do CORS for these resources, I don't think there's any problem at all.  In that situation, the calling site could just XHR to get the same information, so <link rel="prefetch"> would be far *less* risky.
However, it seems a bit odd from an API / protocol design point of view, because you wouldn't want to gate the content load on CORS (because compat), only the firing of the events.

Net of all that, I think if you want to do something now (before we can get more feedback on the risk), CORS is the way to go.
Flags: needinfo?(rlb)
Update the implementation so that cross origin http errors are only reported in CORS case.
Attachment #8770468 - Attachment is obsolete: true
Attachment #8770469 - Attachment is obsolete: true
Attachment #8770470 - Attachment is obsolete: true
Comment on attachment 8775089 [details] [diff] [review]
Add load / error event to prefetch link

Hi Boris,

I've updated the patch so that non-CORS cross origin prefetch should always generate load event. Would you take a look?
Attachment #8775089 - Flags: review?(bzbarsky)
Hmm.  The comments in LoadTainting.h talk about checking the tainting not being adequate, but code inspection suggests it should be ok.  Ben, are those comments still correct after the changes in bug 1226909, or do they need adjusting?
Flags: needinfo?(bkelly)
Since nsContentSecurityManager::CheckChannel() increases the tainting I believe its safe to use whenever AsyncOpen2() has been used to open the channel.
Flags: needinfo?(bkelly)
Alright, sounds like we should adjust those comments.

Samael, I have three more questions about the main logic here:

1)  In the "not cacheable" case, do we really want to fire load instead of error?  Seems like error would be more appropriate in that case, because there is not in fact a usable prefetched thing.  On the other hand, retrying the prefetch isn't going to help either...  I'm a bit torn.

2)  In the "asyncopen fails" case, why is it ok to fire error instead of load?  Why does a failing security check there have different behavior from a failing security check on redirect from an initially-same-origin load?  Or does it?  I guess at this point we know we aren't making any decisions based on what the server returns, so aren't leaking any information about the server to the page, but if that's what makes this ok we should clearly document that reasoning.

3) The current logic for firing events will randomly fire them or not depending on whether a node that the page dropped refs to has had a chance to be gced.  I think we should make this deterministic and only fire the event if domNode->IsInComposedDoc() or some such condition.   That will ensure that there is no gc dependency on event firing, more or less... Does that make sense?  There could still be issues if the node is in a doc that is itself subject to gc (e.g. a createDocument() or responseXML return value), so maybe we want to limit this to cases when the node's composed doc has a docshell or something.
Flags: needinfo?(sawang)
Oh, and lastly, is there a spec issue filed to spec all this?
Hi Samuel 
Comment 23, If you found some timeout failures on try, you may take a look at Bug 1281415.
Prefetching waits for all mochitests are done and probably next prefetch would never be triggered.
Hope this helps
(In reply to Boris Zbarsky [:bz] from comment #27)
> Alright, sounds like we should adjust those comments.
> 
> Samael, I have three more questions about the main logic here:
> 
> 1)  In the "not cacheable" case, do we really want to fire load instead of
> error?  Seems like error would be more appropriate in that case, because
> there is not in fact a usable prefetched thing.  On the other hand, retrying
> the prefetch isn't going to help either...  I'm a bit torn.

The spec was confusing so I wasn't quite sure what the behavior should be, but the mixed-content test cases added "no-cache" header [1], and used load / error events to determine whether a mixed-content request was allowed or blocked. I chose to fire load event since the test cases failed otherwise, but I could be wrong about this.

> 2)  In the "asyncopen fails" case, why is it ok to fire error instead of
> load?  Why does a failing security check there have different behavior from
> a failing security check on redirect from an initially-same-origin load?  Or
> does it?  I guess at this point we know we aren't making any decisions based
> on what the server returns, so aren't leaking any information about the
> server to the page, but if that's what makes this ok we should clearly
> document that reasoning.

My understanding is the same as yours -- at this moment there should be no server response yet so I thought it's OK to generate an error if it's actually blocked. It was also related to the web platform tests, as the test cases expected error events when CSP is block-all-mixed-content. I can't find clear statements in specs to help me decide what I should do so I'm happy to hear more feedbacks.

> 3) The current logic for firing events will randomly fire them or not
> depending on whether a node that the page dropped refs to has had a chance
> to be gced.  I think we should make this deterministic and only fire the
> event if domNode->IsInComposedDoc() or some such condition.   That will
> ensure that there is no gc dependency on event firing, more or less... Does
> that make sense?  There could still be issues if the node is in a doc that
> is itself subject to gc (e.g. a createDocument() or responseXML return
> value), so maybe we want to limit this to cases when the node's composed doc
> has a docshell or something.

I wasn't aware of that. Thanks for reminding.

[1] http://searchfox.org/mozilla-central/source/testing/web-platform/tests/mixed-content/generic/expect.py#100
Flags: needinfo?(sawang)
(In reply to Thomas Nguyen[:tnguyen] (use ni? plz) from comment #29)
> Hi Samuel 
> Comment 23, If you found some timeout failures on try, you may take a look
> at Bug 1281415.
> Prefetching waits for all mochitests are done and probably next prefetch
> would never be triggered.
> Hope this helps

Oh thank you! I'll try to make a workaround for that.
> The spec was confusing so I wasn't quite sure what the behavior should be
....
> I chose to fire load event since the test cases failed otherwise

The mixed-content tests have historically been extremely buggy; they were written by someone who didn't know much about web tech, especially outside Chrome's implementation.  I would not trust them one bit.

I agree the spec, if we're talking about https://w3c.github.io/resource-hints/#load-and-error-events is not very useful in this case.  I filed https://github.com/w3c/resource-hints/issues/62 on this.

For now, I think we should fire "error" here, not "load".

> I can't find clear statements in specs to help me decide what I should do

File bugs on the specs is what you should do.
Comment on attachment 8775089 [details] [diff] [review]
Add load / error event to prefetch link

>+++ b/dom/base/test/resource.sjs
>+ *  - body: get's used as the response body

s/get's/gets/

>+++ b/dom/base/test/test_bug1268962.html

Ideally this would live in web platform tests, but given the lack of spec this is probably fine for now...

>+      document.getElementsByTagName("head")[0].removeChild(link);

How about link.remove(), both places?

>+    document.getElementsByTagName("head")[0].appendChild(link);

  document.head.appendChild(link);

>+.then(() => SimpleTest.finish());

You want a .catch() that will ok(false) with the rejection message and then SimpleTest.finish(), I think...

>+++ b/uriloader/prefetch/nsPrefetchService.cpp
>+                             (loadInfo->GetTainting() == LoadTainting::CORS &&
>+                              NS_SUCCEEDED(mChannel->GetStatus(&rv)) &&
>+                              NS_FAILED(rv));

I think it would be safer to do:

                             (loadInfo->GetTainting() == LoadTainting::CORS &&
                              (NS_FAILED(mChannel->GetStatus(&rv)) ||
                               NS_FAILED(rv)));

so we fall back to the safe thing if GetStatus fails.

>+            // although it's canceled we still want to fire load event

Let's fire error instead.  Though again we may want to condition this on taint state...  Maybe it's simpler to just fire load unconditionally here after all.  :(  I can live with it either way.

>+      nsCOMPtr<nsINode> domNode = do_QueryReferent(node->mSources.ElementAt(i));

As I said above, this should do some checks to make sure we're reasonably deterministic about firing the event.

I'd like to see the updated patch, but this is looking really good.  ;)
Attachment #8775089 - Flags: review?(bzbarsky)
Attachment #8775089 - Attachment is obsolete: true
Comment on attachment 8777700 [details] [diff] [review]
Add load / error event to prefetch link (v2)

Hi Boris,

Would you take a look on the updated patch? It should address those issues you mentioned, and in addition,

1. Add a preference to ignore normal loads for Bug 1281415.

2. Move the CORS checking to the beginning of nsPrefetchNode::OnStartRequest so that if it's a rejected CORS request but with HTTP 200 OK it will set mShouldFireLoadEvent to true.

3. It turns out Android testing is done by running a prebuild version of gecko on host as the test server. It uses Firefox 37 but URLSearchParams's keys(), values(), entries() and for..of support were all implemented in Firefox 44 [1], so I can not use those general access functions. I turned the test server script to be specific for this bug eventually.

[1] https://developer.mozilla.org/zh-TW/docs/Web/API/URLSearchParams
Attachment #8777700 - Attachment description: Add load / error event to prefetch link → Add load / error event to prefetch link (v2)
Attachment #8777700 - Flags: review?(bzbarsky)
Comment on attachment 8777700 [details] [diff] [review]
Add load / error event to prefetch link (v2)

Why the change from using a generic responder to one specific to this test?  The generic responder looked pretty good to me...

>+  SpecialPowers.pushPrefEnv({"clear": [["network.prefetch-next.aggressive"]]}, resolve)))

Why do you need that bit?  The whole point of using pushPrefEnv is that it will reset things back to the original state for you when the test finishes...

>+    // in usual case prefetch does not start until all normal loads are done
>+    // aggressive mode ignores normal loads and just start prefetch ASAP
>+    // it's mainly for testing purpose and discoraged for normal use
>+    // see https://bugzilla.mozilla.org/show_bug.cgi?id=1281415 for details

Please add punctuation and capitalization, like so:

   // Normally prefetch does not start until all normal loads are done.
   // Aggressive mode ignores normal loads and just starts prefetch ASAP.
   // It's mainly for testing purpose and discoraged for normal use;
   // see https://bugzilla.mozilla.org/show_bug.cgi?id=1281415 for details.

r=me with that comment fixed and the pref-clearing bit either explained or removed; your call on whether the responder should be generic or not.

Thank you for doing this!
Flags: needinfo?(igrigorik)
Attachment #8777700 - Flags: review?(bzbarsky) → review+
Attachment #8779196 - Attachment description: Add load / error event to prefetch link → Add load / error event to prefetch link. r=bz
Attachment #8779196 - Flags: review+
Attachment #8777700 - Attachment is obsolete: true
(In reply to Boris Zbarsky [:bz] from comment #39)
> Comment on attachment 8777700 [details] [diff] [review]
> Add load / error event to prefetch link (v2)
> 
> Why the change from using a generic responder to one specific to this test? 
> The generic responder looked pretty good to me...

Unfortunately the generic responder doesn't run on Android tests since the host-utils is still using Firefox 37 with very limited support to URLSearchParams :(
Ugh.  Is there a bug on file to get that updated?  Because that's a pretty big gotcha that people are not likely to notice until they check in....
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/626f8cc8b7bc
Add load / error event to prefetch link. r=bz
Keywords: checkin-needed
(In reply to Boris Zbarsky [:bz] from comment #43)
> Ugh.  Is there a bug on file to get that updated?  Because that's a pretty
> big gotcha that people are not likely to notice until they check in....

Submitted bug 1293952.
https://hg.mozilla.org/mozilla-central/rev/626f8cc8b7bc
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Depends on: 1294449
Depends on: 1294159
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Since it's backed out, let's just merge the patch from bug 1294159.
Attachment #8779196 - Attachment is obsolete: true
Got a fix for that. Waiting for try result. If it looks good I'll mark checkin-needed again.
Flags: needinfo?(sawang)
Comment on attachment 8780409 [details] [diff] [review]
Add load / error event to prefetch link. r=bz

updated patch with fix from bug 1294159
Attachment #8780409 - Attachment description: Add load / error event to prefetch link → Add load / error event to prefetch link. r=bz
Attachment #8780409 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec6e52818ae
Add load / error event to prefetch link. r=bz
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0ec6e52818ae
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
As it is not a recent issue, we can probably let it ride the trains.
status-firefox49: affected → wontfix
status-firefox50: --- → wontfix
platform-rel: --- → ?
Whiteboard: btpp-active → btpp-active [platform-rel-Amazon]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: