Last Comment Bug 696301 - Allow sites to enable x-domain window.onerror information
: Allow sites to enable x-domain window.onerror information
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: mozilla13
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 363897 732209 742549
  Show dependency treegraph
 
Reported: 2011-10-20 19:07 PDT by Robert Kieffer
Modified: 2012-04-05 11:08 PDT (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
part 1. Refactor CORS attribute handling a bit. (16.94 KB, patch)
2012-03-01 14:50 PST, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 2. Communicate the crossorigin preload state from the parser to the scriptloader. (18.93 KB, patch)
2012-03-01 14:51 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 3. Propagate the CORS mode to the script loader correctly; make sure the CORS mode of script preloads matches the actual load. (16.62 KB, patch)
2012-03-01 14:55 PST, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review
part 4. Actually hook up a CORS listener on script loads as needed. (8.06 KB, patch)
2012-03-01 14:55 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 4. Actually hook up a CORS listener on script loads as needed. (8.25 KB, patch)
2012-03-01 15:01 PST, Boris Zbarsky [:bz]
no flags Details | Diff | Splinter Review
part 2. Communicate the crossorigin preload state from the parser to the scriptloader. (20.48 KB, patch)
2012-03-01 20:11 PST, Boris Zbarsky [:bz]
jonas: review+
hsivonen: review+
Details | Diff | Splinter Review
Now with an SVG test (11.16 KB, patch)
2012-03-02 09:25 PST, Boris Zbarsky [:bz]
jonas: review+
Details | Diff | Splinter Review

Description Robert Kieffer 2011-10-20 19:07:01 PDT
We (Facebook) need a mechanism for disabling the window.onerror muting behavior implemented in #363897.  Our static script resources are served on a CDN under a different domain from the main site. Because these domains differ we're falling afoul of the x-domain logic that prevents us from gathering useful information about browser errors.

This "feature" has been widely enough adopted in in the wild (in Firefox and Webkit browsers) that the majority of uncaught exceptions we see in production now have no actionable information in them.

Please add support for a "X-Script-Origin" header that allows sites to specify the domains within which error reporting should be permitted.  For example, we would configure the facebook CDN to include ...

  X-Script-Origin: *.facebook.com

... on all script resources, thereby allowing us to collect error information on any facebook.com subdomain they run in.
Comment 1 Adam Barth 2011-10-21 12:45:57 PDT
Here's what I wrote on the WebKit version of this bug:

"We should just use CORS for this.  The script tag should have a crossorigin attribute like the img tag does.  If the CORS check passes, then we can feel confident in forwarding the errors to the embedding page."

https://bugs.webkit.org/show_bug.cgi?id=70574
Comment 2 Robert Kieffer 2011-10-21 14:19:25 PDT
Agreed - https://developer.mozilla.org/en/CORS_Enabled_Image, but for script tags solves the problem nicely.

(BTW, is there a way to unify the webkit and mozilla tickets for this?  Or should we just continue to cross post? Either way is fine by me.)
Comment 3 Adam Barth 2011-10-21 14:47:36 PDT
You can create an bug in the W3C's HTML issue tracker or just email public-html or whatwg.
Comment 4 Boris Zbarsky [:bz] 2011-10-23 20:05:28 PDT
Yeah, I think comment 1 sounds like a good plan to me.  Jonas, any objections?

In terms of our implementation, do we need a way to flag this "cross origin" bit on the JSScript and JS error report?  Or can we get away with just stamping the page principal as the origin principal (in the bug 624621 sense) on the script if the CORS check passes?
Comment 5 Christopher Blizzard (:blizzard) 2012-02-24 14:24:20 PST
I can't believe I'm about to say this, but "bump."
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-02-25 01:22:43 PST
Did someone email whatwg?
Comment 7 Christopher Blizzard (:blizzard) 2012-02-29 11:37:11 PST
Jonas is discussing options off-bug.  Hoping he will update soon!
Comment 8 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-02-29 12:37:50 PST
The conclusion is to simply use the exactly the same mechanism as we do for <img>. I.e. adding a crossorigin attribute makes us use CORS to do the load. If you set the crossorigin attribute to "use-credentials" we'll use CORS and send credentials.
Comment 9 Boris Zbarsky [:bz] 2012-03-01 14:50:36 PST
Created attachment 602124 [details] [diff] [review]
part 1.  Refactor CORS attribute handling a bit.
Comment 10 Boris Zbarsky [:bz] 2012-03-01 14:51:45 PST
Created attachment 602127 [details] [diff] [review]
part 2.  Communicate the crossorigin preload state from the parser to the scriptloader.
Comment 11 Boris Zbarsky [:bz] 2012-03-01 14:55:00 PST
Created attachment 602130 [details] [diff] [review]
part 3.  Propagate the CORS mode to the script loader correctly; make sure the CORS mode of script preloads matches the actual load.
Comment 12 Boris Zbarsky [:bz] 2012-03-01 14:55:24 PST
Created attachment 602131 [details] [diff] [review]
part 4.  Actually hook up a CORS listener on script loads as needed.
Comment 13 Boris Zbarsky [:bz] 2012-03-01 15:01:59 PST
Created attachment 602136 [details] [diff] [review]
part 4.  Actually hook up a CORS listener on script loads as needed.
Comment 14 Boris Zbarsky [:bz] 2012-03-01 15:02:59 PST
I suppose we also need tests for <svg:script>.... let me know?
Comment 15 Boris Zbarsky [:bz] 2012-03-01 20:11:29 PST
Created attachment 602250 [details] [diff] [review]
part 2.  Communicate the crossorigin preload state from the parser to the scriptloader.
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 02:02:20 PST
(In reply to Boris Zbarsky (:bz) from comment #14)
> I suppose we also need tests for <svg:script>.... let me know?

Yes. :)
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 02:52:57 PST
Comment on attachment 602124 [details] [diff] [review]
part 1.  Refactor CORS attribute handling a bit.

Review of attachment 602124 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsGenericElement.cpp
@@ +6131,5 @@
> +    aResult.ParseEnumValue(aValue, kCORSAttributeTable, false,
> +                           // default value is anonymous if aValue is
> +                           // not a value we understand
> +                           &kCORSAttributeTable[0]);
> +  MOZ_ASSERT(success);

MOZ_ALWAYS_TRUE is good for this case
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 02:55:26 PST
Comment on attachment 602250 [details] [diff] [review]
part 2.  Communicate the crossorigin preload state from the parser to the scriptloader.

Review of attachment 602250 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsDocument.cpp
@@ +7741,5 @@
>      loadFlags |= imgILoader::LOAD_CORS_USE_CREDENTIALS;
> +    break;
> +  default:
> +    /* should never happen */
> +    NS_NOTREACHED("Unknown CORS mode!");

MOZ_NOT_REACHED?

::: content/base/src/nsScriptLoader.cpp
@@ +1326,5 @@
>  
>  void
>  nsScriptLoader::PreloadURI(nsIURI *aURI, const nsAString &aCharset,
> +                           const nsAString &aType,
> +                           const nsAString& aCrossOrigin)

Please put & on a consistent side, preferably on the left.
Comment 19 :Ms2ger (⌚ UTC+1/+2) 2012-03-02 02:59:57 PST
Comment on attachment 602136 [details] [diff] [review]
part 4.  Actually hook up a CORS listener on script loads as needed.

Review of attachment 602136 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/src/nsScriptLoader.cpp
@@ +351,5 @@
> +    listener =
> +      new nsCORSListenerProxy(listener, mDocument->NodePrincipal(), channel,
> +                              withCredentials, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

How about

nsCOMPtr<nsIStreamListener> listener;
if (aRequest->mCORSMode != CORS_NONE) {
  ...
  listener = ... (loader, ... );
  ...
} else {
  listener = loader;
}
Comment 20 Boris Zbarsky [:bz] 2012-03-02 08:26:57 PST
> MOZ_ALWAYS_TRUE is good for this case

In that it lets me skip the DebugOnly bit?  I'd still want the separate boolean, because wrapping MOZ_ALWAYS_TRUE around a long expression seems bad to me.

> MOZ_NOT_REACHED?

Sure.

> How about

Either way.  See what Jonas thinks.
Comment 21 Boris Zbarsky [:bz] 2012-03-02 09:25:14 PST
Created attachment 602390 [details] [diff] [review]
Now with an SVG test
Comment 23 Robert Kieffer 2012-03-10 17:37:05 PST
Boris, will do!
Comment 25 Robert Kieffer 2012-03-12 07:08:26 PDT
Boris/Daniel - Is it expected that crossorigin-scripts will fail to load if the server does not provide the proper Access-Control-Allow-Origin response header?  (This is what I'm currently seeing in my tests.)

Not sure if this is a bug or a feature.  It makes sense given how XHR+CORS behaves (invalid CORS requests behave as if no response was received.)  On the other, it feels inconsistent with the legacy behavior of script tags.
Comment 26 Boris Zbarsky [:bz] 2012-03-12 08:47:06 PDT
> Boris/Daniel - Is it expected that crossorigin-scripts will fail to load if the server
> does not provide the proper Access-Control-Allow-Origin response header?

Yes, that's how CORS behaves.

> On the other, it feels inconsistent with the legacy behavior of script tags.

That's why the CORS behavior has to be opt-in via the attribute, not default for all scripts....
Comment 27 Tobie Langel 2012-03-12 09:05:16 PDT
I second Robert's comment here. This seems like an implementation detail leaking through rather than the expected behavior.

Why not have the script load in all cases and just not provide it's content to error reporting when the headers aren't set properly?

Afaik, cross origin images are still displayed, even if the headers are missing. It's just their pixel data that can;t be reached.

Or am I missing something?
Comment 28 Boris Zbarsky [:bz] 2012-03-12 09:23:49 PDT
> Afaik, cross origin images are still displayed, even if the headers are missing

Not if you add a "crossorigin" attribute to them, no.  If you do that, the load fails.  The argument was that it's better to fail early (and consistently), than to make it look like everything is fine and then break when you try to actually use the image.

Similar for scripts, by the way: people will notice when scripts fail to load in testing, whereas they might miss scripts loading but not exposing error information in testing...

I'm happy to discuss this in the relevant working group, and we can loosen the behavior if that's what's decided on.  Implementing the more loose behavior and then tightening it would obviously be more problematic.
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2012-03-12 11:47:04 PDT
Tobie: For what it's worth, this is the behavior that I discussed with people at facebook and got a go-ahead on.
Comment 30 Tobie Langel 2012-03-12 17:08:28 PDT
> > Afaik, cross origin images are still displayed, even if the headers are missing
> 
> Not if you add a "crossorigin" attribute to them, no.  If you do that, the
> load fails.

Oops. Please ignore my comment. Sorry for the noise.

Whether or not that's a good design choice for CORS as a whole might be up to debate, but this thread really isn't the right place to discuss that.

Anyway, thanks for enabling this!

Note You need to log in before you can comment on or make changes to this bug.