1138781 - Lots of codemirror logspam in every mochitest-dt run

Status: RESOLVED FIXED

(DevTools :: Source Editor, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

In the log below (for example):
http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-debug/1425329849/mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm120-tests1-linux64-build1.txt.gz

I see the following block of warnings repeated a good bit throughout:
14:35:11     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/clike.js, line 263: ReferenceError: reference to undefined property mode.builtin
14:35:11     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/codemirror.js, line 7041: ReferenceError: reference to undefined property arr[(arr.length - 1)]
14:35:11     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/codemirror.js, line 6742: ReferenceError: reference to undefined property options.clearRedo
14:35:11     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/codemirror.js, line 3134: ReferenceError: reference to undefined property cm.display.blinker
14:35:11     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/activeline.js, line 53: ReferenceError: reference to undefined property active[(active.length - 1)]
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/debugger-view.js, line 393: ReferenceError: reference to undefined property this._editorSource.actor
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/codemirror.js, line 834: ReferenceError: reference to undefined property built.bgClass
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/debugger-panes.js, line 270: ReferenceError: reference to undefined property aItem.attachment.actor
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/javascript.js, line 249: ReferenceError: reference to undefined property cc[(cc.length - 1)].lex
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/javascript.js, line 280: ReferenceError: reference to undefined property state.globalVars
14:35:12     INFO -  JavaScript strict warning: chrome://browser/content/devtools/codemirror/javascript.js, line 358: ReferenceError: reference to undefined property cx.state.fatArrowAt

Comment 1

[Ian Moody [:Kwan] (UTC+0)]

Hmm, I can reproduce most/all of these in my regular release Windows install just by opening Scratchpad/DevTools debugger, but can't get them to reproduce at all in my ubuntu VM trunk build.  Not very helpful for testing fixes.  Anyway with most of them being in CM code I don't know if we'd want to modify in place, but perhaps Marijn would be okay with upstream modifications, assuming they don't degrade critical performance.

Anyway here's some rudimentary analysis.

(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #0)
> In the log below (for example):
> http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-linux64-debug/1425329849/mozilla-central_ubuntu64_vm-debug_test-
> mochitest-devtools-chrome-1-bm120-tests1-linux64-build1.txt.gz
> 
> I see the following block of warnings repeated a good bit throughout:
> 14:35:11     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/clike.js, line 263:
> ReferenceError: reference to undefined property mode.builtin
didn;t look, but property existence check I guess
> 14:35:11     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/codemirror.js, line 7041:
> ReferenceError: reference to undefined property arr[(arr.length - 1)]
Happens when a zero length is sent in, resulting in arr[-1]; fixable with just:
-  function lst(arr) { return arr[arr.length-1]; }
+  function lst(arr) { return (arr.length)? arr[arr.length-1] : undefined; }

> 14:35:11     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/codemirror.js, line 6742:
> ReferenceError: reference to undefined property options.clearRedo
    if (options && options.clearRedo !== false)
Could check for the property .clearRedo itself first, or instead add it in line 6993 where this is declared
var sel_dontScroll = {scroll: false}

> 14:35:11     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/codemirror.js, line 3134:
> ReferenceError: reference to undefined property cm.display.blinker
    clearInterval(cm.display.blinker);
could test for existence, or maybe instead add
d.blinker = null;
to the end of Display(), around #240?

> 14:35:11     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/activeline.js, line 53:
> ReferenceError: reference to undefined property active[(active.length - 1)]
Same as before, zero length array resulting in active[-1]
-      if (active[active.length - 1] != line) active.push(line);
+      if (active.length && active[active.length - 1] != line) active.push(line);

> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/debugger-view.js, line 393:
> ReferenceError: reference to undefined property this._editorSource.actor
this is actually mozilla code, but I haven't taken a good look.  Obviously could just check for property existence first

> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/codemirror.js, line 834:
> ReferenceError: reference to undefined property built.bgClass
test for prop existence

> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/debugger-panes.js, line 270:
> ReferenceError: reference to undefined property aItem.attachment.actor
moz code again, test again

> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/javascript.js, line 249:
> ReferenceError: reference to undefined property cc[(cc.length - 1)].lex
I saw this happening when cc[0], maybelabel(type), did't have .lex

> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/javascript.js, line 280:
> ReferenceError: reference to undefined property state.globalVars
> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/codemirror/javascript.js, line 358:
> ReferenceError: reference to undefined property cx.state.fatArrowAt
prop exists checks I guess

Comment 2

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody [:Kwan] from comment #1)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #0)
> > In the log below (for example):
> > http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> > central-linux64-debug/1425329849/mozilla-central_ubuntu64_vm-debug_test-
> > mochitest-devtools-chrome-1-bm120-tests1-linux64-build1.txt.gz
> > 
> > I see the following block of warnings repeated a good bit throughout:
> > 14:35:11     INFO -  JavaScript strict warning:
> > chrome://browser/content/devtools/codemirror/clike.js, line 263:
> > ReferenceError: reference to undefined property mode.builtin
> didn;t look, but property existence check I guess
Actually, I wonder this is better fixed in the defs() below, by giving them
    builtin: {},

Comment 3

[Marijn Haverbeke]

CodeMirror is written in vanilla JavaScript, not the 'strict' dialect that disallows access to nonexistant properties. This is intentional, and is not going to change, so the solution here would be to turn off these warnings.

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Marijn Haverbeke from comment #3)
> CodeMirror is written in vanilla JavaScript, not the 'strict' dialect that
> disallows access to nonexistant properties. This is intentional, and is not
> going to change, so the solution here would be to turn off these warnings.

Ian, does removing the "use strict" line here get rid of most of the warnings for you? https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/codemirror/codemirror.js#18

Comment 5

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Brian Grinstead [:bgrins] from comment #4)
> (In reply to Marijn Haverbeke from comment #3)
> > CodeMirror is written in vanilla JavaScript, not the 'strict' dialect that
> > disallows access to nonexistant properties. This is intentional, and is not
> > going to change, so the solution here would be to turn off these warnings.
> 
> Ian, does removing the "use strict" line here get rid of most of the
> warnings for you?
> https://dxr.mozilla.org/mozilla-central/source/browser/devtools/sourceeditor/
> codemirror/codemirror.js#18
It doesn't seem to have any effect, which surprised me somewhat.

(Given that they don't show in my ubuntu VM, I had to try it out in an old (23rd Jan) Windows debug build I had lying around)

Comment 6

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

I don't think these are normal strict mode errors. They are the SpiderMonkey-specific warnings for things that could in some cases be errors, which are enabled when javascript.options.strict is set to true.

Comment 7

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

(or javascript.options.strict.debug for debug builds)

Comment 8

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Panos Astithas [:past] from comment #6)
> I don't think these are normal strict mode errors. They are the
> SpiderMonkey-specific warnings for things that could in some cases be
> errors, which are enabled when javascript.options.strict is set to true.
Ah, which rather neatly explains why I do/don't observe them in different places.  My regular Windows profile I flipped that pref at some point in the past, presumably for web dev; my Windows debug has the equivalent on automatically; and my ubuntu normal build has it default off.

Unless there is some way of disabling it per-file, this seems WONTFIX then, at least for the CodeMirror ones.
The two in mozilla code could presumably be fixed
> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/debugger-view.js, line 393:
> ReferenceError: reference to undefined property this._editorSource.actor
> 14:35:12     INFO -  JavaScript strict warning:
> chrome://browser/content/devtools/debugger-panes.js, line 270:
> ReferenceError: reference to undefined property aItem.attachment.actor

Comment 9

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ian Moody [:Kwan] from comment #8)
> Unless there is some way of disabling it per-file, this seems WONTFIX then,
> at least for the CodeMirror ones.

Given the giant pile of log bloat this generates, I don't think WONTFIX is an acceptable choice.

Comment 10

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

Apart from WONTFIXing this I can only see 2 options:
- we revisit the recent dev-platform thread: https://groups.google.com/forum/#!topic/mozilla.dev.platform/l7LwHMaCLMk
- we disable that pref by default on our test infrastructure and anyone who sees value in it can still enable it locally

Comment 11

[Brian Grinstead [:bgrins]]

(In reply to Panos Astithas [:past] from comment #10)
> Apart from WONTFIXing this I can only see 2 options:
> - we revisit the recent dev-platform thread:
> https://groups.google.com/forum/#!topic/mozilla.dev.platform/l7LwHMaCLMk
> - we disable that pref by default on our test infrastructure and anyone who
> sees value in it can still enable it locally

Option 2 would either disabling it in prefs_general.js (which would affect all test suites) or adding in a new pref file specific for devtools to disable the logging.

I'll attach a patch that flips the pref just for the devtools subsuite, but looking through that thread I don't see a lot of support for keeping that feature.  Jason, what do you think about removing the javascript.options.strict logging, or at least flipping the default pref to false for 'javascript.options.strict.debug'?

Comment 12

[Brian Grinstead [:bgrins]]

Attachment: prevent-strict-logging-on-devtools-tests.patch

This does option 2 in Comment 10 and just disables the strict logging pref for the devtools subsuite.

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to Brian Grinstead [:bgrins] from comment #11)
> [...] Jason, what do you think about removing the
> javascript.options.strict logging, or at least flipping the default pref to
> false for 'javascript.options.strict.debug'?

Please flip this default to false.

Comment 14

[Brian Grinstead [:bgrins]]

Attachment: strict-logging-false.patch

Just flips the default value

Comment 15

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

Comment on attachment 8576702 [details] [diff] [review]
strict-logging-false.patch

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

How can I express even more approval?!

Comment 16

[Brian Grinstead [:bgrins]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/697c416f8dc6