Closed Bug 962857 Opened 6 years ago Closed 6 years ago

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

Categories

(DevTools :: Debugger, defect, P3)

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 30

People

(Reporter: anton, Assigned: b4bomsy)

Details

Attachments

(2 files, 1 obsolete file)

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.
Yeah, we shouldn't be pretty printing that file by default since it definitely isn't minified.
Summary: Debugger thinks that profiler/controller.js is pretty-printed → Debugger mistakenly thinks that profiler/controller.js is minified and pretty prints it
I'll look into it.
Thanks
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
Yessir!
Assignee: nobody → b4bomsy
Status: NEW → ASSIGNED
Note that this is not just one file, but most of the chrome js(m) files.
Summary: Debugger mistakenly thinks that profiler/controller.js is minified and pretty prints it → Debugger mistakenly thinks that many chrome js(m) files are minified and pretty prints them
(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
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.
(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?
.
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.
(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
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.
Attachment #8368610 - Flags: review?(rcampbell)
Assignee: b4bomsy → past
Attachment #8368610 - Flags: review?(rcampbell) → review+
Back to you Hubert.
Assignee: past → b4bomsy
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
Attached patch 962857fix.patch (obsolete) — Splinter Review
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)
Attachment #8391781 - Flags: review?(nfitzgerald)
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.
Attachment #8391781 - Flags: review?(nfitzgerald) → review+
(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)..
Attached patch 962857fix.patchSplinter Review
Attachment #8391781 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ec77797d0a47
Keywords: checkin-needed
Whiteboard: [leave open] → [leave open][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ec77797d0a47
Whiteboard: [leave open][fixed-in-fx-team] → [leave open]
Any reason for having this as [leave open] still ?
Don't think so!
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → Firefox 30
QA Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.