Allow sites to enable x-domain window.onerror information

RESOLVED FIXED in mozilla13

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Robert Kieffer, Assigned: bz)

Tracking

Trunk
mozilla13
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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

6 years ago
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
(Reporter)

Comment 2

6 years ago
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

6 years ago
You can create an bug in the W3C's HTML issue tracker or just email public-html or whatwg.
Component: General → DOM
OS: Mac OS X → All
Product: Firefox → Core
QA Contact: general → general
Hardware: x86 → All
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

6 years ago
Blocks: 363897
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?
I can't believe I'm about to say this, but "bump."
Did someone email whatwg?
Jonas is discussing options off-bug.  Hoping he will update soon!
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.
Created attachment 602124 [details] [diff] [review]
part 1.  Refactor CORS attribute handling a bit.
Attachment #602124 - Flags: review?(jonas)
Created attachment 602127 [details] [diff] [review]
part 2.  Communicate the crossorigin preload state from the parser to the scriptloader.
Attachment #602127 - Flags: review?(jonas)
Attachment #602127 - Flags: review?(hsivonen)
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.
Attachment #602130 - Flags: review?(jonas)
Created attachment 602131 [details] [diff] [review]
part 4.  Actually hook up a CORS listener on script loads as needed.
Attachment #602131 - Flags: review?(jonas)
Created attachment 602136 [details] [diff] [review]
part 4.  Actually hook up a CORS listener on script loads as needed.
Attachment #602136 - Flags: review?(jonas)
Attachment #602131 - Attachment is obsolete: true
Attachment #602131 - Flags: review?(jonas)
I suppose we also need tests for <svg:script>.... let me know?
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Blocks: 732209
Created attachment 602250 [details] [diff] [review]
part 2.  Communicate the crossorigin preload state from the parser to the scriptloader.
Attachment #602250 - Flags: review?(jonas)
Attachment #602250 - Flags: review?(hsivonen)
Attachment #602127 - Attachment is obsolete: true
Attachment #602127 - Flags: review?(jonas)
Attachment #602127 - Flags: review?(hsivonen)
(In reply to Boris Zbarsky (:bz) from comment #14)
> I suppose we also need tests for <svg:script>.... let me know?

Yes. :)
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 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 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;
}
Attachment #602250 - Flags: review?(hsivonen) → review+
> 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.
Created attachment 602390 [details] [diff] [review]
Now with an SVG test
Attachment #602390 - Flags: review?(jonas)
Attachment #602136 - Attachment is obsolete: true
Attachment #602136 - Flags: review?(jonas)
Attachment #602124 - Flags: review?(jonas) → review+
Attachment #602250 - Flags: review?(jonas) → review+
Attachment #602130 - Flags: review?(jonas) → review+
Attachment #602390 - Flags: review?(jonas) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6429f6f66d
https://hg.mozilla.org/integration/mozilla-inbound/rev/80b48f5c8826
https://hg.mozilla.org/integration/mozilla-inbound/rev/77f754912890
https://hg.mozilla.org/integration/mozilla-inbound/rev/06939c27240a

Thanks for the reviews!

Robert, do you want to give this a try once it lands in a nightly?
Whiteboard: [need review]
Target Milestone: --- → mozilla13

Comment 23

6 years ago
Boris, will do!
https://hg.mozilla.org/mozilla-central/rev/5e6429f6f66d
https://hg.mozilla.org/mozilla-central/rev/80b48f5c8826
https://hg.mozilla.org/mozilla-central/rev/77f754912890
https://hg.mozilla.org/mozilla-central/rev/06939c27240a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Version: unspecified → Trunk

Comment 25

6 years ago
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.
> 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

6 years ago
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?
> 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.
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

6 years ago
> > 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!
Blocks: 742549
You need to log in before you can comment on or make changes to this bug.