1501503 - Send a console warning when masks are blocked due to CORS.

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

[Emilio Cobos Álvarez (:emilio)]

Brad, any chance you could do this? Let me know if not and I'll take a look when I have the time.

This causes a lot of confusion. There's no signal from the browser about anything failing.

Comment 1

[Sean Voisen (:svoisen)]

Setting as P2 since the CORS mask-image fix is riding in 64 (which is now beta). We should definitely show *something* and not fail silently.

Comment 2

[Brad Werth [:bradwerth]]

Attachment: Bug 1501503: Report cross-origin load failures in more cases.

Comment 3

[Brad Werth [:bradwerth]]

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

Comment 4

[Brad Werth [:bradwerth]]

I've posted a patch which makes our CORS failure reporting occur in all cases like this. It's possible that the reporting is now too frequent. If it is, then we'll need to pass more information through to nsCORSListenerProxy::UpdateChannel(), possibly in the nsIChannel for the request.

Comment 5

[Sean Voisen (:svoisen)]

Brad: Thanks for continuing to take on these additional CORS issues.

Comment 6

[Brad Werth [:bradwerth]]

I'm working on a test for this by expanding dom/base/test/test_warning_for_blocked_cross_site_request.html

Comment 7

[Brad Werth [:bradwerth]]

Attachment: Bug 1501503 Part 2: Test that CORS rejection messages are output for loads triggered from styles.

Depends on D9807

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2d9f82adf43
Report cross-origin load failures in more cases. r=ckerschb
https://hg.mozilla.org/integration/autoland/rev/814414676c25
Part 2: Test that CORS rejection messages are output for loads triggered from styles. r=ckerschb

Comment 9

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/c2d9f82adf43
https://hg.mozilla.org/mozilla-central/rev/814414676c25

Comment 10

[Brad Werth [:bradwerth]]

(In reply to Brad Werth [:bradwerth] from comment #4)
> I've posted a patch which makes our CORS failure reporting occur in all
> cases like this. It's possible that the reporting is now too frequent. If it
> is, then we'll need to pass more information through to
> nsCORSListenerProxy::UpdateChannel(), possibly in the nsIChannel for the
> request.

Sightings like Bug 1504284 indicate that we are now over-reporting "errors" for successful loads. Looking at the code again, I see that the modifications in Part 1 are reporting errors in an early-success path through the code. Even if these checks fail, there are other paths that lead to code success. I'll work on identifying those paths.

Cristian, would you please roll this change back?

Comment 11

[Brad Werth [:bradwerth]]

I've found by undoing JUST Part 1 of the patch (the behavior change), the test in Part 2 still passes, indicating that we are reporting CORS messages properly for mask-image without the code changes. Are we sure this is even a bug? With Part 1 undone, when I try the testcase in Bug 1418470 (try to load https://test.shhnjk.com/CSSMask.html), I also correctly get a CORS error message.

Emilio, do you have another testcase that was failing? I'd like to try it with Part 1 undone to confirm this is actually a bug.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yes,

  mozregression --launch 2018-10-15 -a https://bugzilla.mozilla.org/attachment.cgi?id=9019544

(The date is fairly arbitrary, but enough so that it doesn't contain this patch).

That shows no CORS warning at all. Shows a CSP error, but that's due to it being hosted in BMO, if you run the test locally you'll see no warnings at all.

https://test.shhnjk.com/CSSMask.html also shows no warning at all in that Nightly.

I've also tested 2018-10-29 with same results, fwiw.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

There's a chance that bug 1496505 fixed this somehow? I don't know.

Comment 14

[Cristian Brindusan [:cbrindusan]]

Brad, can you please let me know if this changeset needs a backout (or both of them)? 
https://hg.mozilla.org/integration/autoland/rev/c2d9f82adf43

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6860806fc1f
Backed out 2 changesets as requested by bradwerth on the ticket.

Comment 16

[Brad Werth [:bradwerth]]

Thank you. Backing out both changesets is the right thing to do. The test will possibly be added back in another bug. Closing this bug again because the original issue no longer results in a silent failure.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/abe6431b9950
Part 2: Test that CORS rejection messages are output for loads triggered from styles. r=ckerschb

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I relanded the test because I think it's worth not to regress this accidentally.

Comment 19

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/abe6431b9950