Distinguish dynamically generated <script> elements

RESOLVED FIXED in Firefox 66

Status

defect
P2
normal
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

(Blocks 1 bug)

unspecified
Firefox 66
Dependency tree / graph

Firefox Tracking Flags

(firefox66 fixed)

Details

Attachments

(5 attachments, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
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.
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.
Attachment #9033895 - Flags: review?(jorendorff)
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.
Attachment #9033897 - Flags: review?(jlaster)
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 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. (?)
Attachment #9033895 - Flags: review?(jorendorff)
(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.
> 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.
Attachment #9033897 - Flags: review?(jlaster) → review?(lsmyth)
(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 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.
Attachment #9033897 - Flags: review?(lsmyth)
Posted patch patchSplinter Review
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.
Attachment #9033894 - Attachment is obsolete: true
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
Attachment #9034089 - Flags: review?(jorendorff)
Updated patch now that introductionScript is set properly for dynamically generated script elements.
Attachment #9033895 - Attachment is obsolete: true
Attachment #9033897 - Attachment is obsolete: true
Attachment #9034091 - Flags: review?(lsmyth)
Attachment #9034089 - Flags: review?(jorendorff) → review+
Priority: -- → P2
Attachment #9034091 - Flags: review?(lsmyth) → review+
Blocks: dbg-sources
After running on try, several devtools tests need updating to be compatible with this change.
Attachment #9034494 - Flags: review?(lsmyth)
Attachment #9034494 - Flags: review?(lsmyth) → review+
Pushed by bhackett@mozilla.com:
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.
You need to log in before you can comment on or make changes to this bug.