1010150 - Can't pretty-print inline scripts

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Hallvord R. M. Steen [:hallvors]]

Attachment: prettyme.htm

1) Load attached test
2) Select it in "Debugger"
3) Click "Prettify source" button

Bug: the all-on-one-line inline script isn't reformatted.

This is a big problem for debugging certain important sites - Google Maps is but one example.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

(By reading bug 833245 I could not quite figure out if it was about this issue - if I've reported a dup please close it accordingly)

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

Bug 833245 is about the same problem, but with a different tool. It's not very likely that the solution could be shared as they use different libraries for pretty-printing.

Comment 3

[Panos Astithas (he/him) [:past] (please ni?)]

One possible solution could be for the script actor to use the inspector actor to get to the page's script tag, get its text content, pass it to the pretty-printer and then replace the editor's content with the result.

Mike or Brian, does this sound feasible with the current inspector actor(s)? Will that be easier than the script actor doing its own markup parsing?

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #3)
> One possible solution could be for the script actor to use the inspector
> actor to get to the page's script tag, get its text content, pass it to the
> pretty-printer and then replace the editor's content with the result.
> 
> Mike or Brian, does this sound feasible with the current inspector actor(s)?
> Will that be easier than the script actor doing its own markup parsing?

Sure, with a nodefront and walker you can get the inner HTML of a tag.  It would be possible to add a method for getting prettified text for a node as well (possibly even attached to the nodefront).  That won't be a problem - the hard part will be (a) how to get a nodefront from the script actor in the first place, and (b) knowing where to replace the text in the editor.  I'm not sure how the HTML of the page is grabbed for listing in the debugger in the first place, but I wonder if it would be easier to either:

1) Use js beautify, which I think could prettify the entire text (including HTML)
2) Implement a getPrettifiedHTML on the nodefront that is intended to be called on the document node, recurses over all nodes, appending the node.outerHTML if it isn't a script, or prettify(node.outerHTML) if it is a script.  I'm not sure how (if at all) we would work with source maps in this case.

I think approach 1 would be quite easy to implement (although it doesn't support source map generation at all AFAIK).


Somewhat relevant script you could run this in a browser scratchpad on the page:

let {devtools} = Cu.import("resource://gre/modules/devtools/Loader.jsm", {});
let tt = devtools.TargetFactory.forTab(gBrowser.selectedTab);
let toolbox = gDevTools._toolboxes.get(tt);
let walker = toolbox.inspector.walker;

walker.getRootNode().then(root => {
  walker.querySelectorAll(root, "script").then(nodelist => {
    nodelist.items().then(nodes => {
     for (let n of nodes) {
      walker.innerHTML(n).then(str=> {
        console.log("Got inner HTML", str);
      });
     }
    });
  });
});

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Panos Astithas [:past] from comment #3)
> > One possible solution could be for the script actor to use the inspector
> > actor to get to the page's script tag, get its text content, pass it to the
> > pretty-printer and then replace the editor's content with the result.
> > 
> > Mike or Brian, does this sound feasible with the current inspector actor(s)?
> > Will that be easier than the script actor doing its own markup parsing?
> 
> Sure, with a nodefront and walker you can get the inner HTML of a tag.  It
> would be possible to add a method for getting prettified text for a node as
> well (possibly even attached to the nodefront).  That won't be a problem -
> the hard part will be (a) how to get a nodefront from the script actor in
> the first place, and (b) knowing where to replace the text in the editor. 
> I'm not sure how the HTML of the page is grabbed for listing in the debugger
> in the first place, but I wonder if it would be easier to either:

As for (a), I guess it will be easy if you have access to the DOMNode from the script actor, with WalkerActor.attachElement

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> 1) Use js beautify, which I think could prettify the entire text (including
> HTML)

We could use beautify.html(), which will tidy HTML, CSS and JS but we really don't need to tidy the HTML.

Using beautify.css() and beautify.js() would do the job though.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Michael Ratcliffe [:miker] [:mratcliffe] from comment #6)
> (In reply to Brian Grinstead [:bgrins] from comment #4)
> > 1) Use js beautify, which I think could prettify the entire text (including
> > HTML)
> 
> We could use beautify.html(), which will tidy HTML, CSS and JS but we really
> don't need to tidy the HTML.
> 
> Using beautify.css() and beautify.js() would do the job though.

If I understand correctly: unless if beatify.js() would generate source maps then we would not be able to debug the script after prettifying since all of the character positions would change.

Comment 8

[Panos Astithas (he/him) [:past] (please ni?)]

(In reply to Brian Grinstead [:bgrins] from comment #7)
> If I understand correctly: unless if beatify.js() would generate source maps
> then we would not be able to debug the script after prettifying since all of
> the character positions would change.

This is true, but that is already the case with breakpoints and pretty printing. I think we have a bug on file to fix this, but I can't find it at the moment. Nick, what do you think?

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Panos Astithas [:past] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #7)
> > If I understand correctly: unless if beatify.js() would generate source maps
> > then we would not be able to debug the script after prettifying since all of
> > the character positions would change.
> 
> This is true, but that is already the case with breakpoints and pretty
> printing. I think we have a bug on file to fix this, but I can't find it at
> the moment. Nick, what do you think?

You can totally debug pretty printed scripts, however there is a bug where we need to update the locations of breakpoints that were already set. Bug 952612.

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that setting new breakpoints does work.

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> (In reply to Panos Astithas [:past] from comment #8)
> > (In reply to Brian Grinstead [:bgrins] from comment #7)
> > > If I understand correctly: unless if beatify.js() would generate source maps
> > > then we would not be able to debug the script after prettifying since all of
> > > the character positions would change.
> > 
> > This is true, but that is already the case with breakpoints and pretty
> > printing. I think we have a bug on file to fix this, but I can't find it at
> > the moment. Nick, what do you think?
> 
> You can totally debug pretty printed scripts, however there is a bug where
> we need to update the locations of breakpoints that were already set. Bug
> 952612.

So it sounds like it might be best to be consistent by generating a source map like we do with other scripts so that when bug 952612 gets fixed the breakpoint locations will be preserved just the same for scripts in HTML as they are for external scripts.

What's still unclear to me is it seems like there is only one SourceActor created for the entire page (even if it has more than one script on it).  This actor is where all of the pretty printing / source map stuff is handled.  So (a) should there be a new source actor built for each inline script on a page, (b) should the pretty printing move into some other construct that could support multiple inline scripts on an HTML source actor, or (c) is there some other way to tackle this?

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> (In reply to Nick Fitzgerald [:fitzgen] from comment #9)
> > (In reply to Panos Astithas [:past] from comment #8)
> > > (In reply to Brian Grinstead [:bgrins] from comment #7)
> > > > If I understand correctly: unless if beatify.js() would generate source maps
> > > > then we would not be able to debug the script after prettifying since all of
> > > > the character positions would change.
> > > 
> > > This is true, but that is already the case with breakpoints and pretty
> > > printing. I think we have a bug on file to fix this, but I can't find it at
> > > the moment. Nick, what do you think?
> > 
> > You can totally debug pretty printed scripts, however there is a bug where
> > we need to update the locations of breakpoints that were already set. Bug
> > 952612.
> 
> So it sounds like it might be best to be consistent by generating a source
> map like we do with other scripts so that when bug 952612 gets fixed the
> breakpoint locations will be preserved just the same for scripts in HTML as
> they are for external scripts.
> 
> What's still unclear to me is it seems like there is only one SourceActor
> created for the entire page (even if it has more than one script on it). 
> This actor is where all of the pretty printing / source map stuff is
> handled.  So (a) should there be a new source actor built for each inline
> script on a page, (b) should the pretty printing move into some other
> construct that could support multiple inline scripts on an HTML source
> actor, or (c) is there some other way to tackle this?

We are moving to (a) in the process of using Debugger.Source in the debugger server: bug 905700

Comment 13

[Brian Grinstead [:bgrins]]

OK, seems that waiting for 905700 would be the most straightforward thing to do in this case.  Prettifying  on the frontend would be easy with js beautify, but this would surely break debugging

Comment 14

[Dan Callahan [:callahad]]

Adding DevAdvocacy tag. I wasn't able to debug a real-world problem in Firefox because of this (a user on MetaFilter wanted to know how a site was displaying an overlay whenever she started to switch tabs: http://ask.metafilter.com/280379/Stop-popping-up-when-Im-about-to-switch-the-tab).

I switched to Chrome and was able to debug this.

Comment 15

[Karl Dubost💡 :karlcow]

Many of the "One page web app" requires this. 
For example, one recent case I had is finding Web Compatibility issues in the code of Gmail, which is basically a giant HTML file containing scripts and CSS.

We need a way to beautify them. 
Currently, I'm copying the full document in SublimeText and apply an HTML beautifier that will 

1. reflow the HTML
2. reflow the CSS
3. reflow the JS


But then I lose the ability to insert breakpoints, etc. 

The Style Editor shows inline CSS. 
The debugger could do it in a similar fashion.

Also careful, some scripts are inserted with (example source code of gmail)

<script>
/*

blah.

*/
</script>

Comment 16

[Mike Taylor [:miketaylr]]

Normally I just use Chrome as a workaround for this use case, or in a pinch use Charles proxy to serve a beautified script on my filesystem... but today I'm stuck trying to debug some touch event handlers that are minified inline. :(

(Using Chrome doesn't help because it works in Chrome, and the site is HTTPS which makes working with Charles Proxy cumbersome and prone to not work).

https://github.com/webcompat/web-bugs/issues/1039#issuecomment-181194494

Comment 17

[Hallvord R. M. Steen [:hallvors]]

Crude workaround, Mike:

arguments.callee.toString() in console shows the function you're inside, stringified. You can do replace(/;/g, ';\n') for some basic pretty-printing.

Comment 18

[Hallvord R. M. Steen [:hallvors]]

Now in 47a1 8ef94be995a453f5c464278c53478ba8c8554f81 it seems something changed for the worse here: when trying to see an inline script nothing appears at all and there's an error in the browser console:

A promise chain failed to handle a rejection. Did you forget to '.catch', or did you forget to 'return'?
See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise

Date: Wed Mar 02 2016 22:44:49 GMT+0100
Full Message: Error: Can't prettify non-javascript files.
Full Stack: togglePrettyPrint/</<@resource://gre/modules/commonjs/toolkit/loader.js -> resource://devtools/client/debugger/content/actions/sources.js:140:17

Comment 19

[Patrick Brosset <:pbro>]

In the new debugger, the pretty print button does not appear in this case at all. 
I moved this to the issues on the debugger repo: https://github.com/devtools-html/debugger.html/issues/4887

Comment 20

[:Harald Kirschner :digitarald]

… and back from GH to bugzilla, pardon the noise.

Comment 24

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The test case from comment #0 is also online here
http://janodvarko.cz/tests/bugzilla/1010150/

The Debugger currently doesn't show the Prettify button in this case at ll.

Honza

Comment 25

[Alexandre Poirot [:ochameau]]

Note that, as part of going through bug 1569859, I'm seeing various issues around inline scripts.
So:

1. I may fix issues which are blockers to implement this
2. I will certainly grow knowledges around inline scripts, which are having some convoluted treatment by the thread actor.

Comment 26

[Alexandre Poirot [:ochameau]]

Here is some early analysis to understand the challenge here.

How pretty print is enabled?

The pretty print button is being disabled for inline script from here:
https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/devtools/client/debugger/src/reducers/sources.js#843-859

 const sourceContent =
    source.content && isFulfilled(source.content) ? source.content.value : null;

  if (!sourceContent || !isJavaScript(source, sourceContent)) {
    return false;
  }

isJavaScript(source, sourceContent) returns false, because the source's content type is "text/html".
This is checked over here:
https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/devtools/client/debugger/src/utils/source.js#79

If we relax this first check, pretty printing is also blocked just before trying to pretty print over here:
https://searchfox.org/mozilla-central/rev/016925857e2f81a9425de9e03021dcf4251cafcc/devtools/client/debugger/src/actions/sources/prettyPrint.js#42-61

  if (!isJavaScript(generatedSource, content) || content.type !== "text") {
    throw new Error("Can't prettify non-javascript files.");
  }

  const url = getPrettyOriginalSourceURL(generatedSource);
  const { code, mappings } = await prettyPrint({
    text: content.value,
    url,
  });

If we also relax this check, prettyPrint() method throws as it really only accepts Javascript code and isn't designed to process HTML with inline <script> tags.

What happens in the frontend regarding inline scripts

With this simple example, loading 3 trivial inline <script>'s in the base HTML file:
http://techno-barje.fr/fission/inlines/
We end up with one redux source (i.e. devtools/client/debugger/src/reducers/sources.js):

 {
  "id": "source-http://techno-barje.fr/fission/inlines/",
  "url": "http://techno-barje.fr/fission/inlines/",
  "relativeUrl": "/fission/inlines/",
  "isPrettyPrinted": false,
  "extensionName": null,
  "isBlackBoxed": false,
  "isWasm": false,
  "isExtension": false,
  "isOriginal": false,
  "content": {
    "state": "fulfilled",
    "value": {
      "type": "text",
      "value": "\nInline scripts\n\n<script>\nconsole.log(\"first inline\");\n</script>\n\n<script>\nconsole.log(\"second inline\");\n</script>\n\n<script>\nconsole.log(\"third inline\");\n</script>\n",
      "contentType": "text/html"
    }
  }
}

But three distinct redux source actors (i.e. devtools/client/debugger/src/reducers/source-actors.js)

[
  {
    "id": "server0.conn1.windowGlobal6442450946/source22",
    "actor": "server0.conn1.windowGlobal6442450946/source22",
    "thread": "server0.conn1.windowGlobal6442450946/thread1",
    "source": "source-http://techno-barje.fr/fission/inlines/",
    "isBlackBoxed": false,
    "sourceMapBaseURL": "http://techno-barje.fr/fission/inlines/",
    "sourceMapURL": null,
    "url": "http://techno-barje.fr/fission/inlines/",
    "introductionType": "scriptElement"
  },
  {
    "id": "server0.conn1.windowGlobal6442450946/source23",
    "actor": "server0.conn1.windowGlobal6442450946/source23",
    "thread": "server0.conn1.windowGlobal6442450946/thread1",
    "source": "source-http://techno-barje.fr/fission/inlines/",
    "isBlackBoxed": false,
    "sourceMapBaseURL": "http://techno-barje.fr/fission/inlines/",
    "sourceMapURL": null,
    "url": "http://techno-barje.fr/fission/inlines/",
    "introductionType": "scriptElement"
  },
  {
    "id": "server0.conn1.windowGlobal6442450946/source24",
    "actor": "server0.conn1.windowGlobal6442450946/source24",
    "thread": "server0.conn1.windowGlobal6442450946/thread1",
    "source": "source-http://techno-barje.fr/fission/inlines/",
    "isBlackBoxed": false,
    "sourceMapBaseURL": "http://techno-barje.fr/fission/inlines/",
    "sourceMapURL": null,
    "url": "http://techno-barje.fr/fission/inlines/",
    "introductionType": "scriptElement"
  }
]

This is where it starts being complex. How do we fetch the source content?
From the server we can only fetch javascript source content via SourceActors.
Do we fetch each individual inline script and somehow merge them?
No.
source.content.value is actually arbitrarily fetched from the first source-actor able to respond:
https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/actions/sources/loadSourceText.js#61-87

  // We only need the source text from one actor, but messages sent to retrieve
  // the source might fail if the actor has or is about to shut down. Keep
  // trying with different actors until one request succeeds.
  let response;
  const handledActors = new Set();
  while (true) {
    const actors = getSourceActorsForSource(state, source.id);
    const actor = actors.find(({ actor: a }) => !handledActors.has(a));
    handledActors.add(actor.actor);
    try {
      response = await client.sourceContents(actor);
      break;
    } catch (e) {
      console.warn(`sourceContents failed: ${e}`);
    }
  }

  return {
    text: response.source,
    contentType: response.contentType || "text/javascript",
  };

(Note that this code looks uterly complex for no reason. getSourceActorsForSource should hopefully be called only once... unless we workaround some weird race condition that should be fixed by some other mean)

From the server perspective, we don't return spidermonkey's Source's text attribute, which is restricted to the Javascript code of only one given inline <script> tag.
Instead we return the full HTML page content. We load Spidermonkey's Source's url, which is the html page:
https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/server/actors/source.js#211-227

    if (this._source.text !== "[no source]" && !this._isInlineSource) {
      return {
        content: this.actualText(),
        contentType: "text/javascript",
      };
    }

    return this.sourcesManager.urlContents(
      this.url,
      /* partial */ false,
      /* canUseCache */ this._isInlineSource
    );

I think this is where the confusion starts as we still interact with each individual source actor.

Review of all the frontend code interacting with many source actors

• exceptions:
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/reducers/exceptions.js#55-78
  But at the end, I'm not sure we care about which particular inline script the exception relates to.
  We may simplify that by matching per URL...

• blackbox:
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/actions/sources/blackbox.js#17-19
  But this doesn't look relevant as SourceActor.blackbox method ends up calling the shared SourcesManager:
  https://searchfox.org/mozilla-central/source/devtools/server/actors/source.js#610-611
  So... until the SourceActor's URL are differents or the SourceActors run in distinct threads, we should be able to call it only once, on the ThreadActor... as that's not per SourceId/SourceActor, but per URL!
  (Also, we may not use the returned value of this RDP method...)

• pretty print
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/actions/sources/loadSourceText.js#41-45
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/actions/sources/prettyPrint.js#53-57
  The source map generated by pretty fast is being "applied" to all the source actors.
  For now, I don't follow what it means when there is more than one actor.

• breakable lines
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/actions/sources/breakableLines.js#44-50
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/server/actors/source.js#261-262
  This is where we start the scratch the full complexity of inline script.
  There is a quite convoluted logic on the server, in SourceActor, dedicated to inline scripts.
  This is there to understand where the related inline <script> tag is in the html page.
  This is what _adjustInlineScriptLocation is about:
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/server/actors/source.js#340-359
  I will ignore the underlying issues of this particular method (which is async and brittle).
  We then apply some shift between what Spidermonkey's Script.getPossibleBreakpoints() returns and what we report back to the frontend:
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/server/actors/source.js#544-552
  Note that this is also used when setting breakpoints:
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/server/actors/source.js#708-715

• make breakpoint
  https://searchfox.org/mozilla-central/rev/633345116df55e2d37be9be6555aa739656c5a7d/devtools/client/debugger/src/utils/breakpoint/index.js#45-48
  This code makes no sense to me.
  Could we simply do breakpointLocation.sourceId = source.id?

Comment 27

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1010150 - [devtools] Add Debugger.Source.startColumn. r=jandem.

This will help us getting the actual text of an inline script in DevTools

Depends on D170581

Comment 28

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 1010150 - [devtools] Allow to pretty-print inline scripts. r=#devtools-reviewers.

Depends on D170750

Comment 29

[Phabricator Automation]

Comment on attachment 9319450 [details]
Bug 1010150 - [devtools] Add Debugger.Source.startColumn. r=jandem.

Revision D170750 was moved to bug 1819290. Setting attachment 9319450 [details] to obsolete.

Comment 30

[Nicolas Chevobbe [:nchevobbe]]

Note that the event listener popup only reference the formatted file with Bug 1557196 applied

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1b825d22f20b
[devtools] Allow to pretty-print inline scripts. r=devtools-reviewers,ochameau.

Comment 32

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/1b825d22f20b

Comment 33

[Ryan VanderMeulen [:RyanVM]]

Do you want to add this to the Fx113 relnotes?

Comment 34

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #33)

Do you want to add this to the Fx113 relnotes?

Not sure, for now we only pretty print the content of inline script in the HTML, and not the HTML itself, which people might think is a bug. I'd rather make an announcement when/if we pretty print the html as well