Last Comment Bug 508562 - Add option to disable javascript.options.strict for debug builds
: Add option to disable javascript.options.strict for debug builds
Status: RESOLVED FIXED
[firebug-p1]
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: John J. Barton
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-08-05 08:01 PDT by John J. Barton
Modified: 2010-02-24 06:30 PST (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
508562-lessStrict.diff (2.78 KB, patch)
2009-08-06 17:03 PDT, John J. Barton
bzbarsky: review+
jst: superreview+
Details | Diff | Splinter Review

Description John J. Barton 2009-08-05 08:01:11 PDT
I just learned from Boris today Firefox debug builds force javascript.options.strict on all chrome js. Since Firebug is not 'strict', it emits huge numbers of warnings, making the performance of Firebug on debug builds horrific.

I propose that http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp be changed to add
  javascript.options.strict.debug
which defaults to true. The runtime value of this pref would control the block of code which forces the strict setting:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#1251

In non-debug builds, there is one additional pref, never used.

In debug builds, the default causes the current behavior.

In debug builds, setting strict.debug false would skip the force block.
Comment 1 John J. Barton 2009-08-05 08:03:18 PDT
I put this in DOM component only because the source path to nsJSEnvironment had "/dom/".

I posted the above to moz.dev.platform since it effects every developer potentially.
Comment 2 Boris Zbarsky [:bz] 2009-08-05 11:50:37 PDT
ccing some UI-like folks for their feedback, since this primarily affects them.
Comment 3 Dão Gottwald [:dao] 2009-08-05 12:58:25 PDT
(In reply to comment #0)
> Since Firebug is not 'strict', it
> emits huge numbers of warnings, making the performance of Firebug on debug
> builds horrific.

Another pref doesn't seem to solve that problem adequately, unless you expect everyone to cheerfully disable strict warnings.

I've personally enabled strict warnings explicitly.
Comment 4 John J. Barton 2009-08-05 13:12:11 PDT
(In reply to comment #3)
> Another pref doesn't seem to solve that problem adequately, unless you expect
> everyone to cheerfully disable strict warnings.
 
Sorry I did not explain this part.

My proposal is to set the pref when Firebug is installed/started. That way everyone else is left as they are today.
Comment 5 Dão Gottwald [:dao] 2009-08-05 13:30:04 PDT
Do you mean /you/ would set the pref when starting Firebug, or Firebug would do it automatically? Both solutions seem to ignore those who'd like to get strict warnings for their code, leaving them either with no strict warnings or with the performance issue and lots of Firebug noise.
Comment 6 John J. Barton 2009-08-05 13:39:03 PDT
(In reply to comment #5)
> Do you mean /you/ would set the pref when starting Firebug, or Firebug would do
> it automatically? 

Firebug would set the strict.debug pref automatically, so the Mozilla developers running debug builds with Firebug installed would not have 'strict' on during that particular session.

> Both solutions seem to ignore those who'd like to get strict
> warnings for their code, leaving them either with no strict warnings or with
> the performance issue and lots of Firebug noise.

This bug says nothing one way or another about that problem, which is a different one. I do think we need a better approach to controlling 'strict' and I would be happy to contribute to a discussion of that if you choose to open another bug on that subject.

To reiterate, this bug says nothing about 'strict' on normal builds or debug builds without firebug. I think its a serious problem, just not this problem.
Comment 7 John J. Barton 2009-08-06 11:59:36 PDT
I have made the source changes for this on my machine. Now I want to build in way that will set DEBUG and that will give more confidence about the patch. So I want to be sure about my .mozconfig.  Here is what I normally use (a symbols only build):

ac_add_options --disable-vista-sdk-requirements
ac_add_options --enable-application=browser
ac_add_options --disable-optimize 
ac_add_options --disable-parental-controls
ac_add_options --disable-libxul
mk_add_options MOZ_DEBUG_SYMBOLS=1
ac_add_options --enable-debugger-info-modules=yes
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

I guess I want:

ac_add_options --disable-vista-sdk-requirements
ac_add_options --enable-application=browser
ac_add_options --disable-optimize 
ac_add_options --disable-parental-controls
ac_add_options --disable-libxul
ac_add_options --enable-debug  
mk_add_options MOZ_OBJDIR=@TOPSRCDIR@/obj-@CONFIG_GUESS@

In addition I also normally set 
export MOZ_DEBUG_SYMBOLS=1
which I don't have now, but if there is something else I need please tell me.
Comment 8 John J. Barton 2009-08-06 17:03:59 PDT
Created attachment 393068 [details] [diff] [review]
508562-lessStrict.diff 

Proposed patch. Compiles but I can't test because the build fails for some unrelated reason.  I'll try again eventually.

Two files need to be changed, nsJSEnvironment.cpp and and modiles/libpref/src/init/all.js. 

All code in the patch is with #ifdef DEBUG, so it should not affect release builds.
Comment 9 Rob Campbell [:rc] (:robcee) 2009-08-10 07:54:41 PDT
wouldn't it be easier to just fix the strict warnings in Firebug? Most of them are due to accessing undefined variables. I fixed a few in a test patch for another bug. I plan on pushing these back up to Firebug when I've disentangled the bits.
Comment 10 John J. Barton 2009-08-10 20:04:36 PDT
(In reply to comment #9)
> wouldn't it be easier to just fix the strict warnings in Firebug? 

There is quite a bit on this issue here:
http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/13acadc848bf612d#

This fix is simple and likely to be effective.  If I could only test it....
Comment 11 Rob Campbell [:rc] (:robcee) 2009-08-11 09:32:59 PDT
well, maybe so. Fixing up the trailing comma problem (not one I think we run into a lot in Firebug's warnings) is going to be a help.

Other than that I've got fixes for about three strict warnings I was running into a lot of the time. I'll get those submitted to fb1.5 by end of day today.
Comment 12 John J. Barton 2009-08-11 11:46:17 PDT
I succeeded in building m-c. I ran the result and verified the javascript.options.strict messages are seen.  I also see an infinite loop caused by a warning in errors.js which triggers itself.

Then I changed Firebug branches/firebug1.5 at R3928 to add 
pref("javascript.options.strict.debug", false);

Re-running the same as before showed no javascript options strict messages. The load was much faster (though still much slower than the symbols-only build).

The Firebug pref will be released in Firebug 1.5a21; I'll post here at that time. 

This is all I can do from here, hopefully someone can take it forward.
Comment 13 John J. Barton 2009-08-11 14:16:40 PDT
(In reply to comment #12)
>  I also see an infinite loop
> caused by a warning in errors.js which triggers itself.

I tried to track this down 
reference to undefined property this[arguments[0]] xpcsafejsobjectwrapper.cpp line 591.
The string "this[arguments[0]]" does not appear in Firebug. I set a breakpoint on 591, but I could not get it to hit because I started getting to many asserts and I got tried of telling MSVC++ to move on. I won't open bugs on these since I can't tell you how to reproduce them.
Comment 14 John J. Barton 2009-09-10 08:57:46 PDT
(In reply to comment #12)

Firebug 1.5a21+ supports:
> pref("javascript.options.strict.debug", false);
...
> This is all I can do from here, hopefully someone can take it forward.

What next?
Comment 15 Rob Campbell [:rc] (:robcee) 2009-10-28 12:13:13 PDT
you have to submit your patch for review to one of the peers in DOM. Then you'll need super review before this can get checked in.

peers are: Olli Pettay, Jonas Sicking, Boris Zbarsky
Comment 16 Boris Zbarsky [:bz] 2009-11-30 22:16:57 PST
Comment on attachment 393068 [details] [diff] [review]
508562-lessStrict.diff 

Looks ok, but jst should give this a once-over
Comment 17 Boris Zbarsky [:bz] 2009-12-22 16:52:33 PST
Pushed http://hg.mozilla.org/mozilla-central/rev/c11e04dd702f
Comment 18 John J. Barton 2009-12-22 21:56:15 PST
Thanks! I'll check it in 2 weeks when I return.

Note You need to log in before you can comment on or make changes to this bug.