Fix some JS warnings in Cleopatra profiler

RESOLVED FIXED in Firefox 29

Status

()

Firefox
Developer Tools: Performance Tools (Profiler/Timeline)
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: cpeterson, Assigned: cpeterson)

Tracking

unspecified
Firefox 29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
Created attachment 8355049 [details] [diff] [review]
part-1-warnings.patch

Enabling pref javascript.options.strict=true (or when running any debug build of Firefox, where javascript.options.strict.debug is true by default) reports a number of JS warnings (in the Browser Console, not the Web Console) when running the Cleopatra profiler:

TypeError: function TreeView__getReverseSelectionSnapshot does not always return a value tree.js:158
TypeError: variable queryData redeclares argument ui.js:1885
ReferenceError: assignment to undeclared variable gLightMode devtools.js:98
ReferenceError: assignment to undeclared variable gVideoCapture ui.js:1451
ReferenceError: reference to undefined property this._initSelection tree.js:132
ReferenceError: reference to undefined property this._allowNonContigous ui.js:275
ReferenceError: reference to undefined property snapshot[0] tree.js:181
TypeError: gSampleBar is null ui.js:496

* In tree.js, TreeView checks `this._initSelection === true` to select _horizontalScrollbox.firstChild, but _initSelection is never initialized to true. Is this dead code or should _initSelection be initialized elsewhere?

* In ui.js, HistogramView_histogramClick() uses gSampleBar, which is always undefined because gSampleBar is never initialized!
Attachment #8355049 - Flags: review?(rcampbell)
(Assignee)

Comment 1

4 years ago
Created attachment 8355050 [details] [diff] [review]
part-2-contiguous.patch

Part 2: Fix misspellings of "contiguous" (and initialize ProfileTreeManager._allowNonContiguous = false).
Attachment #8355050 - Flags: review?(rcampbell)
(Assignee)

Comment 2

4 years ago
Created attachment 8355051 [details] [diff] [review]
part-3-chmod.patch

Part 3: chmod u-x *.js

Change some Cleopatra .js files' modes from rwx------ to rw------- (removing the executable permission).
Attachment #8355051 - Flags: review?(rcampbell)
Comment on attachment 8355049 [details] [diff] [review]
part-1-warnings.patch

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

I'm generally fine with these changes, but this stuff was imported from the gecko profiler addon's codebase. I'm going to defer to Anton who made our local changes for ultimate review on these.

::: browser/devtools/profiler/cleopatra/js/devtools.js
@@ -94,5 @@
>   * in the light mode and creates all the UI we need.
>   */
>  function initUI() {
>    gHideSourceLinks = getParam("ext") === "true";
> -  gLightMode = true;

unused?

::: browser/devtools/profiler/cleopatra/js/parserWorker.js
@@ +265,5 @@
>      rawProfile.profileJSON.meta = rawProfile.meta;
>    }
>  
>    if (typeof rawProfile == "object") {
> +    var format = rawProfile.format || null;

would rather see s/var/let/ here but I guess this is file-consistent.
Attachment #8355049 - Flags: review?(rcampbell)
Attachment #8355049 - Flags: review?(anton)
Attachment #8355049 - Flags: feedback+
Comment on attachment 8355050 [details] [diff] [review]
part-2-contiguous.patch

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

::: browser/devtools/profiler/cleopatra/js/tree.js
@@ -192,5 @@
>            currNode = currNode.treeChildren[i];
>            continue next_level;
>          }
>        }
> -      if (allowNonContigious === true) {

I have no idea why anyone would do this. :)

::: browser/devtools/profiler/cleopatra/js/ui.js
@@ +210,5 @@
>    },
>    restoreSerializedSelectionSnapshot: function ProfileTreeManager_restoreSerializedSelectionSnapshot(selection) {
>      this._savedSnapshot = JSON.parse(selection);
>    },
> +  _restoreSelectionSnapshot: function ProfileTreeManager__restoreSelectionSnapshot(snapshot, allowNonContiguous) {

much signature. wow.
Attachment #8355050 - Flags: review?(rcampbell) → review?(anton)
Comment on attachment 8355051 [details] [diff] [review]
part-3-chmod.patch

why the mode changes?
Attachment #8355051 - Flags: review?(rcampbell)
(Assignee)

Comment 6

4 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #5)
> Comment on attachment 8355051 [details] [diff] [review]
> part-3-chmod.patch
> 
> why the mode changes?

A number of files in the browser/devtools/profiler/cleopatra/ directories are marked as executable (in .html and .svg files). Do we care? It is a little strange. This is probably an artifact when the files were imported from upstream.
(Assignee)

Comment 7

4 years ago
(In reply to Rob Campbell [:rc] (:robcee) from comment #3)
> ::: browser/devtools/profiler/cleopatra/js/devtools.js
> @@ -94,5 @@
> >   * in the light mode and creates all the UI we need.
> >   */
> >  function initUI() {
> >    gHideSourceLinks = getParam("ext") === "true";
> > -  gLightMode = true;
> 
> unused?

gLightMode and gVideoCapture are set, but never declared or read:

https://mxr.mozilla.org/mozilla-central/search?string=gLightMode
https://mxr.mozilla.org/mozilla-central/search?string=gVideoCapture
Comment on attachment 8355049 [details] [diff] [review]
part-1-warnings.patch

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

This looks fine except for removal of "use strict" everywhere. Is there a good reason for that?

::: browser/devtools/profiler/cleopatra/js/ProgressReporter.js
@@ +41,5 @@
>   * duration for the subtask. This value will be used as a weight when
>   * calculating the progress of the parent reporter.
>   */
>  
> +"use strict";

Was strict mode generating warnings?

::: browser/devtools/profiler/cleopatra/js/devtools.js
@@ -94,5 @@
>   * in the light mode and creates all the UI we need.
>   */
>  function initUI() {
>    gHideSourceLinks = getParam("ext") === "true";
> -  gLightMode = true;

According to dxr, it's unused yeah.

::: browser/devtools/profiler/cleopatra/js/parserWorker.js
@@ +265,5 @@
>      rawProfile.profileJSON.meta = rawProfile.meta;
>    }
>  
>    if (typeof rawProfile == "object") {
> +    var format = rawProfile.format || null;

Why can't it be `switch (rawProfile.format || null)`?
Comment on attachment 8355050 [details] [diff] [review]
part-2-contiguous.patch

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

LGTM.
Attachment #8355050 - Flags: review?(anton) → review+
(Assignee)

Comment 10

4 years ago
(In reply to Anton Kovalyov (:anton, @valueof) from comment #8)
> Comment on attachment 8355049 [details] [diff] [review]
> part-1-warnings.patch
> 
> Review of attachment 8355049 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks fine except for removal of "use strict" everywhere. Is there a
> good reason for that?

This patch is adding "use strict", not removing it. :)
Comment on attachment 8355049 [details] [diff] [review]
part-1-warnings.patch

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

This looks fine except for removal of "use strict" everywhere. Is there a good reason for that?

UPDATE: I'm an idiot.

::: browser/devtools/profiler/cleopatra/js/ProgressReporter.js
@@ +41,5 @@
>   * duration for the subtask. This value will be used as a weight when
>   * calculating the progress of the parent reporter.
>   */
>  
> +"use strict";

Was strict mode generating warnings?

::: browser/devtools/profiler/cleopatra/js/devtools.js
@@ -94,5 @@
>   * in the light mode and creates all the UI we need.
>   */
>  function initUI() {
>    gHideSourceLinks = getParam("ext") === "true";
> -  gLightMode = true;

According to dxr, it's unused yeah.

::: browser/devtools/profiler/cleopatra/js/parserWorker.js
@@ +265,5 @@
>      rawProfile.profileJSON.meta = rawProfile.meta;
>    }
>  
>    if (typeof rawProfile == "object") {
> +    var format = rawProfile.format || null;

Why can't it be `switch (rawProfile.format || null)`?
Attachment #8355049 - Flags: review?(anton) → review+
I also don't know how to use Bugzilla.
(Assignee)

Comment 13

4 years ago
Landed patch 1 and 2 with Anton's `switch (rawProfile.format || null)` suggestion:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d462e39beb7c
https://hg.mozilla.org/integration/mozilla-inbound/rev/d79dc7a1e94b

Ignoring chmod patch 3.
https://hg.mozilla.org/mozilla-central/rev/d462e39beb7c
https://hg.mozilla.org/mozilla-central/rev/d79dc7a1e94b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.