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

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

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?

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

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

So the spec here is vague 
https://w3c.github.io/resource-hints/#load-and-error-events

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

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

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.

Comment 6

[ericsc]

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Richard, this too is blocked on <https://github.com/whatwg/html/issues/1142>....

Comment 9

[u408661]

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

Comment 10

[Boris Zbarsky [:bzbarsky]]

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

Comment 11

[Andrew Overholt [:overholt]]

I'm exploring a few options.

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 13

[Boris Zbarsky [:bzbarsky]]

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?

Comment 14

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 1: Add load / error event to prefetch link

Comment 15

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 2: Enable prefetch web-platform-tests

Comment 16

[Samael Wang [:freesamael] (away for now)]

Attachment: Part 3: Expect fail for no-opt-in test cases

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 17

[Samael Wang [:freesamael] (away for now)]

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?

Comment 18

[Boris Zbarsky [: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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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?

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Richard Barnes [:rbarnes]]

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.

Comment 22

[Samael Wang [:freesamael] (away for now)]

Attachment: Add load / error event to prefetch link

Update the implementation so that cross origin http errors are only reported in CORS case.

Comment 23

[Samael Wang [:freesamael] (away for now)]

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

Comment 24

[Samael Wang [:freesamael] (away for now)]

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?

Comment 25

[Boris Zbarsky [: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?

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Since nsContentSecurityManager::CheckChannel() increases the tainting I believe its safe to use whenever AsyncOpen2() has been used to open the channel.

Comment 27

[Boris Zbarsky [:bzbarsky]]

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.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Oh, and lastly, is there a spec issue filed to spec all this?

Comment 29

[Thomas Nguyen (:tnguyen)]

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

Comment 30

[Samael Wang [:freesamael] (away for now)]

(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

Comment 31

[Samael Wang [:freesamael] (away for now)]

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

Comment 32

[Boris Zbarsky [:bzbarsky]]

> 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 33

[Boris Zbarsky [:bzbarsky]]

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.  ;)

Comment 34

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3164c134007d

Comment 35

[Samael Wang [:freesamael] (away for now)]

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

Comment 36

[Samael Wang [:freesamael] (away for now)]

Attachment: Add load / error event to prefetch link (v2)

Comment 37

[Samael Wang [:freesamael] (away for now)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3771931ae7c6

Comment 38

[Samael Wang [:freesamael] (away for now)]

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

Comment 39

[Boris Zbarsky [: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!

Comment 40

[Samael Wang [:freesamael] (away for now)]

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

Comment 41

[Samael Wang [:freesamael] (away for now)]

Attachment: Add load / error event to prefetch link. r=bz

Comment 42

[Samael Wang [:freesamael] (away for now)]

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

Comment 43

[Boris Zbarsky [:bzbarsky]]

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

Comment 44

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/626f8cc8b7bc
Add load / error event to prefetch link. r=bz

Comment 45

[Samael Wang [:freesamael] (away for now)]

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

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/626f8cc8b7bc

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for causing bug 1294449: https://hg.mozilla.org/mozilla-central/rev/233ab21b64b5

Comment 48

[Samael Wang [:freesamael] (away for now)]

Attachment: Add load / error event to prefetch link. r=bz

Since it's backed out, let's just merge the patch from bug 1294159.

Comment 49

[Samael Wang [:freesamael] (away for now)]

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

Comment 50

[Samael Wang [:freesamael] (away for now)]

Got a fix for that. Waiting for try result. If it looks good I'll mark checkin-needed again.

Comment 51

[Samael Wang [:freesamael] (away for now)]

Comment on attachment 8780409 [details] [diff] [review]
Add load / error event to prefetch link. r=bz

updated patch with fix from bug 1294159

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec6e52818ae
Add load / error event to prefetch link. r=bz

Comment 53

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/0ec6e52818ae

Comment 54

[Sylvestre Ledru [:Sylvestre]]

As it is not a recent issue, we can probably let it ride the trains.