962857 - Debugger mistakenly thinks that many chrome js(m) files are minified and pretty prints them

Status: RESOLVED FIXED

(DevTools :: Debugger, defect, P3)

Description

[Anton Kovalyov (:anton)]

If you open Browser Toolbox > Debugger and select profiler/controller.js, Debugger thinks that the source if minified and tries to pretty-print it. Since the source has ES6 constructions in it, it fails producing invalid code.

Comment 1

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

Yeah, we shouldn't be pretty printing that file by default since it definitely isn't minified.

Comment 2

[Hubert B Manilla]

I'll look into it.
Thanks

Comment 3

[Hubert B Manilla]

i think increasing the SAMPLE_SIZE will fix this, we are using 30 lines as the sample size ATM and because those 30 lines are mostly declaration statements (therefore no indents) the heuristics fails.

Pls could this assign this to me, So it comes on my bugmotodo list to fix? thanks

Comment 4

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

Yessir!

Comment 5

[Girish Sharma [:Optimizer]]

Note that this is not just one file, but most of the chrome js(m) files.

Comment 6

[Hubert B Manilla]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> Note that this is not just one file, but most of the chrome js(m) files.
Yeah i noticed, most of these files have the same structure. thanks

Comment 7

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

Perhaps looking at the last n lines would give us better results than looking at the first n lines and wouldn't require iterating over more of each source.

Comment 8

[Hubert B Manilla]

(In reply to Nick Fitzgerald [:fitzgen] from comment #7)
> Perhaps looking at the last n lines would give us better results than
> looking at the first n lines and wouldn't require iterating over more of
> each source.
That's a tot? 
There is also the situation to consider where the file might contain only declarations.
I'm thinking of adding a check on the line length as well. What do think?
.

Comment 9

[Anton Kovalyov (:anton)]

What about comparing the average length of a line with the average line count? Minified files have less lines but each one of them is longer than usual.

Comment 10

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

(In reply to Anton Kovalyov (:anton) from comment #9)
> What about comparing the average length of a line with the average line
> count? Minified files have less lines but each one of them is longer than
> usual.

Probably a reliable hueristic, but the downside is that we would have to look over the whole source instead of just a small constant number of lines in it.

Would also have to be careful of the edge cases that break median and mode line length I described here: https://bugzilla.mozilla.org/show_bug.cgi?id=913665#c2

Comment 11

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

Attachment: Disable auto-pretty-print by default until we fix the heuristics

We are branching on Monday and too many people are affected. Let's disable auto-pretty-print by default for now until we fix this.

Comment 12

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

https://hg.mozilla.org/integration/fx-team/rev/777869c35c0d

Comment 13

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

Back to you Hubert.

Comment 14

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

The pref change broke the auto-pretty-print tests, because they assumed the pref would be enabled by default. I pushed a change to make them work regardless of the actual default value:

https://hg.mozilla.org/integration/fx-team/rev/82e30c1856ee

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/777869c35c0d
https://hg.mozilla.org/mozilla-central/rev/82e30c1856ee

Comment 16

[Hubert B Manilla]

Attachment: 962857fix.patch

This patch fixes the false detection of .jsm files as minified and handles files which may not have indents but are not actually minified (e.g files containing just variable declarations)

Comment 17

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

Comment on attachment 8391781 [details] [diff] [review]
962857fix.patch

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

LGTM! Fix up the nits, reattach, and mark "checkin-needed"!

::: browser/devtools/debugger/debugger-panes.js
@@ +8,5 @@
>  
>  // Used to detect minification for automatic pretty printing
> +const SAMPLE_SIZE = 50; // no of lines
> +const INDENT_COUNT_THRESHOLD = 5; // percentage
> +const CHARACTER_LIMIT = 250; // line character limit 

Nit: trailing space.

@@ +1513,5 @@
>        }
>        if (/^\s+/.test(aText.slice(lineStartIndex, lineEndIndex))) {
>          indentCount++;
>        }
> +      // For files with no indents but are not minified. 

Nit: trailing space.

@@ +1515,5 @@
>          indentCount++;
>        }
> +      // For files with no indents but are not minified. 
> +      if ((lineEndIndex - lineStartIndex) > CHARACTER_LIMIT) {
> +        overCharLimit = true;

We can break iteration here as well because if a single line is over the character limit we consider the source minified. No need to keep checking other lines.

Comment 18

[Hubert B Manilla]

(In reply to Nick Fitzgerald [:fitzgen] from comment #17)
> Comment on attachment 8391781 [details] [diff] [review]
> 962857fix.patch
> 
> Review of attachment 8391781 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM! Fix up the nits, reattach, and mark "checkin-needed"!
> 
> ::: browser/devtools/debugger/debugger-panes.js
> @@ +8,5 @@
> >  
> >  // Used to detect minification for automatic pretty printing
> > +const SAMPLE_SIZE = 50; // no of lines
> > +const INDENT_COUNT_THRESHOLD = 5; // percentage
> > +const CHARACTER_LIMIT = 250; // line character limit 
> 
> Nit: trailing space.
> 
> @@ +1513,5 @@
> >        }
> >        if (/^\s+/.test(aText.slice(lineStartIndex, lineEndIndex))) {
> >          indentCount++;
> >        }
> > +      // For files with no indents but are not minified. 
> 
> Nit: trailing space.
> 
> @@ +1515,5 @@
> >          indentCount++;
> >        }
> > +      // For files with no indents but are not minified. 
> > +      if ((lineEndIndex - lineStartIndex) > CHARACTER_LIMIT) {
> > +        overCharLimit = true;
> 
> We can break iteration here as well because if a single line is over the
> character limit we consider the source minified. No need to keep checking
> other lines.

Thanks.. nick. 
My war against trailing spaces, started it has (yoda voice)..

Comment 19

[Hubert B Manilla]

Attachment: 962857fix.patch

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/ec77797d0a47

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec77797d0a47

Comment 22

[Girish Sharma [:Optimizer]]

Any reason for having this as [leave open] still ?

Comment 23

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

Don't think so!