Closed Bug 1010150 Opened 10 years ago Closed 1 year ago

Can't pretty-print inline scripts

Categories

(DevTools :: Debugger, enhancement, P2)

Product:
DevTools
Component:
Debugger
Platform:
Other
Other
Type:
enhancement

Tracking

(firefox113 fixed)

Status:
RESOLVED FIXED
Milestone:
113 Branch
Tracking Flags:
Tracking Status
firefox113 --- fixed

People

(Reporter: hsteen, Assigned: nchevobbe)

References

(Blocks 3 open bugs, Regressed 1 open bug,
URL
)

Details

(Keywords: DevAdvocacy, Whiteboard: [polish-backlog][DevRel:P1], dt-webcompat, [devtools:relnote])

Attachments

(2 files, 1 obsolete file)

prettyme.htm
973 bytes, text/html
Details
Bug 1010150 - [devtools] Allow to pretty-print inline scripts. r=#devtools-reviewers.
48 bytes, text/x-phabricator-request
Details | Review
Attached file 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.
(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)
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.
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?
(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);
      });
     }
    });
  });
});
(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
(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.
(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.
(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?
Flags: needinfo?(nfitzgerald)
(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.
Flags: needinfo?(nfitzgerald)
Note that setting new breakpoints does work.
(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?
Flags: needinfo?(nfitzgerald)
(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
Flags: needinfo?(nfitzgerald)
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
Depends on: dbg-source
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.
Keywords: DevAdvocacy
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>
Whiteboard: [polish-backlog]
Priority: -- → P1
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
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.
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
See Also: → 1267518
Whiteboard: [polish-backlog] → [polish-backlog][DevRel:P1]
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
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
Product: Firefox → DevTools

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

Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Type: defect → enhancement
Priority: P1 → P2

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

Whiteboard: [polish-backlog][DevRel:P1] → [polish-backlog][DevRel:P1], dt-webcompat

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.

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

Severity: normal → S3
Assignee: nobody → nchevobbe
Status: REOPENED → ASSIGNED
Summary: Can't pretty-print / deobfuscate / beautify inline scripts → Can't pretty-print inline scripts

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

Depends on D170581

Attachment #9319450 - Attachment description: Bug 1010150 - [devtools] Add Debugger.Source.column. r=jandem. → Bug 1010150 - [devtools] Add Debugger.Source.startColumn. r=jandem.
Attachment #9319451 - Attachment description: Bug 1010150 - [devtools] Allow to pretty-print inline scripts. r=devtools-reviewers. → Bug 1010150 - [devtools] Allow to pretty-print inline scripts. r=#devtools-reviewers.

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.

Attachment #9319450 - Attachment is obsolete: true

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

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

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

Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 1 year ago
status-firefox113: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 113 Branch

Do you want to add this to the Fx113 relnotes?

Flags: needinfo?(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

Flags: needinfo?(nchevobbe)
Blocks: 1825208
Whiteboard: [polish-backlog][DevRel:P1], dt-webcompat → [polish-backlog][DevRel:P1], dt-webcompat, [devtools:relnote]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: