1387473 - add test for cross-domain source map loading

Status: RESOLVED FIXED

(DevTools :: General, enhancement, P3)

Description

[Tom Tromey :tromey]

In https://github.com/devtools-html/devtools-core/issues/551 I plan to
change the source map worker to use fetch rather than XHR.
There Jason said that this wasn't done to avoid a CORS problem; so
first I plan to land a cross-domain source map test.

Comment 1

[Tom Tromey :tromey]

Attachment: Bug 1387473 - add cross-domain source-map test;

Review commit: https://reviewboard.mozilla.org/r/164956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164956/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8893867 [details]
Bug 1387473 - add cross-domain source-map test;

https://reviewboard.mozilla.org/r/164956/#review170362

Thanks for the patch! 

I have a few background questions about what this is really exercizing.
We have a bundle on domain A, and a sourcemap on domain B and we want to check that we can fetch the sourcemap on domain B without any issue.
I guess it's a reasonable thing to check, but as devtools running in the panel, I assume we are fetching the sourcemap using the system principal?
Am I correct here, or do we ask for the sourcemap on behalf of the content page?

Now the reason I'm asking is that you mentioned you discussed it with Jason, and I assume the constraints are different when running inside of a browser tab compared to running in Firefox? Maybe that's why an issue with CORs was brought up? (and in this case I'm not sure having this test will help prevent breakage?)

::: devtools/client/framework/test/browser_source_map-cross-domain.js:32
(Diff revision 1)
> +  // Start with the empty page, then navigate, so that we can properly
> +  // listen for new sources arriving.

Is this comment relevant here?

Comment 3

[Tom Tromey :tromey]

Comment on attachment 8893867 [details]
Bug 1387473 - add cross-domain source-map test;

https://reviewboard.mozilla.org/r/164956/#review170362

> Am I correct here, or do we ask for the sourcemap on behalf of the content page?

You're correct.  The source map is fetched by a worker running in chrome context.

> I assume the constraints are different when running inside of a browser tab compared to running in Firefox? Maybe that's why an issue with CORs was brought up? (and in this case I'm not sure having this test will help prevent breakage?)

The browser tab case actually already uses fetch.  See https://github.com/devtools-html/devtools-core/blob/master/packages/devtools-utils/src/network-request.js -- AFAIK only the M-C worker uses privileged-network-request.

So, I don't think that can be the issue.

It's not entirely clear what the issue might be.  All I know is: https://github.com/devtools-html/devtools-core/issues/551#issuecomment-320062687

So I thought perhaps testing the basic case would be a good preventative measure.

I hope this helps.  Honestly I'm not certain that this is either necessary or sufficient.  I welcome other suggestions.

Comment 4

[Julian Descottes [:jdescottes]]

Thanks for the clarifications Tom. I agree this is still worth testing, just was unsure about the conversation on GitHub :)

Comment 5

[Tom Tromey :tromey]

Comment on attachment 8893867 [details]
Bug 1387473 - add cross-domain source-map test;

https://reviewboard.mozilla.org/r/164956/#review170362

> Is this comment relevant here?

Indeed not.

Comment 6

[Tom Tromey :tromey]

Comment on attachment 8893867 [details]
Bug 1387473 - add cross-domain source-map test;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164956/diff/1-2/

Comment 7

[Tom Tromey :tromey]

Those eslint failures were from an earlier patch that was on the branch, and they're fixed
in the patch that landed.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/50efa6725f5d
add cross-domain source-map test; r=jdescottes

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/50efa6725f5d