1517167 - Distinguish dynamically generated <script> elements

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P2)

Description

[Brian Hackett [Laid off!]]

Attachment: patch

The debugger treats <script> elements that are part of an HTML page's source in the same way as <script> elements that were dynamically generated: it is possible for JS to create a script element and fill it with whatever text it wants.

This is a problem because script element sources associated with the HTML page are displayed in the debugger as if they are part of the HTML page itself --- text and line numbers and so forth are supposed to match up with the contents of the HTML page.  For a dynamically generated script element, this will not be true.  This is visible in the debugger in a couple ways:

- When the debugger stops in one of these scripts, the paused location is not shown correctly.

- If the script has a source map, the debugger will ignore it and not show the file included in the mapping --- the script's source is conflated with others for the HTML page itself.

The attached patch fixes these issues by keeping track of which ScriptSources are dynamically generated, and using this in the devtools server and client to treat dynamically generated <script> element sources like eval() sources so that they are not treated as part of the HTML page.  The dynamically generated script is still associated with its original URL via the introductionUrl property so that source maps can be processed correctly.

This fixes both https://github.com/devtools-html/debugger.html/issues/4373 and https://github.com/devtools-html/debugger.html/issues/6706.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Add DebuggerSource.hasScriptOrigin

Add an accessor DebuggerSource.hasScriptOrigin to indicate whether the underlying source was created while a same-realm script was running.  This is a reasonable approximation to the property we want here --- that the source was created dynamically instead of as a static part of an HTML page --- but it isn't a perfect one.  A script could be dynamically generated in one realm by code running as part of another same-origin realm, in which case there would be a cross-realm access involved and .hasScriptOrigin would be false.  Making this hack more complicated doesn't seem great either, though, and it's better to err on the side of not marking a script as dynamically generated.

Ideally the DOM could indicate whether a source was dynamically vs. statically generated in its creation options, but I don't see a way to distinguish those cases without making some substantial changes to the calling DOM code.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Watch for dynamically generated <scripts> in devtools server/client.

The devtools server needs some changes to use DebuggerSource.hasScriptOrigin to distinguish dynamically vs. statically generated script element sources and avoid conflating the former with the HTML page itself.  Some changes to the devtools client were also needed, as the dynamically generated source will not have a .url property, breaking the source mapping library.

Comment 3

[Brian Hackett [Laid off!]]

Attachment: Part 3 - Add test.

Comment 4

[Logan Smyth [:loganfsmyth]]

Comment on attachment 9033897 [details] [diff] [review]
Part 2 - Watch for dynamically generated <scripts> in devtools server/client.

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

::: devtools/client/debugger/new/src/actions/sources/newSources.js
@@ +92,5 @@
> +      if (!urlInfo.url) {
> +        // If the source was dynamically generated (via eval, dynamically
> +        // created script elements, and so forth), it might not have a URL as
> +        // required by getOriginalURLs. Pick a URL to use here.
> +        urlInfo.url = urlInfo.introductionUrl || "http://unknown";

Since we'll never find anything for "http://unknown" can we instead explicitly set `urls = [];`?

I'm also not clear on the use of introductionUrl here. If the `introductionUrl` is the value to use, why wouldn't we set `url` to the right value in `form()`?

::: devtools/server/actors/utils/TabSources.js
@@ +239,5 @@
>      // script element, or does not have a src attribute.
>      const element = source.element ? source.element.unsafeDereference() : null;
>      if (element && (element.tagName !== "SCRIPT" || !element.hasAttribute("src"))) {
> +      if (source.hasScriptOrigin) {
> +        spec.isGeneratedInlineSource = true;

Looking at the logic, it seems like we might be able to do `spec.contentType = "application/javascript";` here to avoid the need for a new flag?

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 9033895 [details] [diff] [review]
Part 1 - Add DebuggerSource.hasScriptOrigin

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

I think you can get this bit by looking at source.introductionScript. It's undefined if there was no script on the stack when the source was created.

For details, search for "introductionScript" in here: <https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Source>

Btw, documentation for new Debugger features goes in js/src/doc/Debugger/*.md.

Clearing review, I think we can do without this. (?)

Comment 6

[Brian Hackett [Laid off!]]

(In reply to Logan Smyth [:loganfsmyth] from comment #4)
> Comment on attachment 9033897 [details] [diff] [review]
> Part 2 - Watch for dynamically generated <scripts> in devtools server/client.
> 
> Review of attachment 9033897 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/debugger/new/src/actions/sources/newSources.js
> @@ +92,5 @@
> > +      if (!urlInfo.url) {
> > +        // If the source was dynamically generated (via eval, dynamically
> > +        // created script elements, and so forth), it might not have a URL as
> > +        // required by getOriginalURLs. Pick a URL to use here.
> > +        urlInfo.url = urlInfo.introductionUrl || "http://unknown";
> 
> Since we'll never find anything for "http://unknown" can we instead
> explicitly set `urls = [];`?

There doesn't seem to be any fetch done for urlInfo.url --- AFAICT it is used to construct a full URL if the source map info includes relative paths.  (Where is the documentation/source for the source map worker code, btw?  The in-tree stuff seems to already have run through webpack.)  Setting urlInfo.url to "http://unknown" here still allows things to work, at least on the react todomvc page (https://github.com/devtools-html/debugger.html/issues/4373).  In the sources pane the mapped sources will show up as relative to an 'unknown' domain instead of the original page, which is why we use introductionUrl here (the url of the page which dynamically created the script element) when available.

> I'm also not clear on the use of introductionUrl here. If the
> `introductionUrl` is the value to use, why wouldn't we set `url` to the
> right value in `form()`?

The client collapses together sources with the same .url, regardless of whether they have the same contents.  A null .url is used for evaluated sources (which is what these dynamically generated script elements essentially are), which the client won't collapse.

> ::: devtools/server/actors/utils/TabSources.js
> @@ +239,5 @@
> >      // script element, or does not have a src attribute.
> >      const element = source.element ? source.element.unsafeDereference() : null;
> >      if (element && (element.tagName !== "SCRIPT" || !element.hasAttribute("src"))) {
> > +      if (source.hasScriptOrigin) {
> > +        spec.isGeneratedInlineSource = true;
> 
> Looking at the logic, it seems like we might be able to do `spec.contentType
> = "application/javascript";` here to avoid the need for a new flag?

Sure, I'll see if this flag can be removed.

Comment 7

[Logan Smyth [:loganfsmyth]]

> In the sources pane the mapped sources will show up as relative to an 'unknown' domain instead of the original page, which is why we use introductionUrl here (the url of the page which dynamically created the script element) when available.

This is the core of my question. If we don't have an introduction URL, we essentially don't know anything about the map location. I guess my question was whether we should actually show the sources in that case. It might make sense to set `urls = [];` and log a message about not having an base URL for the source map load. Showing `unknown` in the sidebar is one way, but to me it seems not ideal.

> The client collapses together sources with the same .url, regardless of whether they have the same contents.

Yeah nevermind. Looks like `getOriginalURLs` just behaves in a way I didn't expect.

Comment 8

[Brian Hackett [Laid off!]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment on attachment 9033895 [details] [diff] [review]
> Part 1 - Add DebuggerSource.hasScriptOrigin
> 
> Review of attachment 9033895 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you can get this bit by looking at source.introductionScript. It's
> undefined if there was no script on the stack when the source was created.
> 
> For details, search for "introductionScript" in here:
> <https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Source>
> 
> Btw, documentation for new Debugger features goes in
> js/src/doc/Debugger/*.md.
> 
> Clearing review, I think we can do without this. (?)

Unfortunately, source.introductionScript is undefined for the scripts of interest here.  It looks like it is only set for eval'ed scripts and 'new Function()' and the like.

This could be fixed, but at this point I'm feeling like another approach might be needed for this bug.  The issues in comment 0 are a manifestation of a more general problem --- it should not be possible for the debugger to display incorrect contents for source tabs, at least when source maps are not involved.  The server knows what script text was compiled and ran, and if the debugger shows something different (an HTML document the script was inappropriately treated as embedded in, an out-of-date HTML document, another source that has the same URL, ...) it will be incredibly confusing for the user.  If we reorient things so that the source's text (or a hash of it) is the basis for comparing different sources, or checking if a source came from within an HTML document, then we should be able to avoid all of these problems, and have a simple argument that the debugger is showing correct sources to users.

If the source map is not correct it doesn't seem like much can be done, except maybe giving users an easy way to toggle between the original and generated sources.

Comment 9

[Brian Hackett [Laid off!]]

Comment on attachment 9033897 [details] [diff] [review]
Part 2 - Watch for dynamically generated <scripts> in devtools server/client.

Sorry for the churn, I think I'm going to rewrite this patch per comment 8.

Comment 10

[Brian Hackett [Laid off!]]

Attachment: patch

Updated patch.  After some discussion the plan now is to land this stuff first (it does, after all, fix the problems we know about wrt dynamically generated scripts with source maps not showing up in the debugger) and pursue a more complete fix in followup.  This patch sets introduction information for sources of script elements that were dynamically generated by another script, and the devtools server uses this to add special handling for such scripts in a couple places.

This reduces the diff size, but has an effect on the todomvc react page: at the call to getOriginalURLs in the client, the introduction URL is now (correctly) the place where the script element injection was performed instead of the place which it was injected into.  The .jsx files then end up with the rather bogus and unwieldy pathname of todomvc.com/examples/react/node_modules/react/dist/examples/react/js.

This could be fixed now, but it would require adding yet another URL to the source information sent up to the client; an alternative would be to wait until the more complete fix is ready, at which point we should be able to associate the page's URL with all its sources (including eval'ed ones) without worrying that the client will collapse them together.

Comment 11

[Brian Hackett [Laid off!]]

Attachment: Part 1 - Set introduction information for dynamically generated <script> elements.

Set introduction information for sources of dynamically created script elements, so that introductionScript and other DebuggerSource properties work on them.  This fixes behavior to match the documentation in https://developer.mozilla.org/en-US/docs/Tools/Debugger-API/Debugger.Source

Comment 12

[Brian Hackett [Laid off!]]

Attachment: Part 2 - Watch for dynamically generated <scripts> in devtools server/client.

Updated patch now that introductionScript is set properly for dynamically generated script elements.

Comment 13

[Brian Hackett [Laid off!]]

Attachment: Part 4 - Test changes.

After running on try, several devtools tests need updating to be compatible with this change.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0c9a3c8405d
Part 1 - Set introduction information for dynamically generated <script> elements, r=jorendorff.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f674573e791b
Part 2 - Watch for dynamically generated <scripts> in devtools server/client, NOT REVIEWED YET.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c64a12a9b285
Part 3 - Add test for source maps in dynamically generated <script> elements.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1760e005cecc
Part 4 - Test fixes for introduction information in dynamically generated <script> elements, r=lsmyth.

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/b0c9a3c8405d
https://hg.mozilla.org/mozilla-central/rev/f674573e791b
https://hg.mozilla.org/mozilla-central/rev/c64a12a9b285
https://hg.mozilla.org/mozilla-central/rev/1760e005cecc