Closed Bug 696301 Opened 13 years ago Closed 12 years ago

Allow sites to enable x-domain window.onerror information

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla13

People

(Reporter: broofa, Assigned: bzbarsky)

References

Details

Attachments

(4 files, 3 obsolete files)

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.
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
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.)
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
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.
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
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.
Attachment #602390 - Flags: review?(jonas)
Attachment #602136 - Attachment is obsolete: true
Attachment #602136 - Flags: review?(jonas)
Boris, will do!
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....
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.
> > 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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.