Closed Bug 508562 Opened 15 years ago Closed 15 years ago

Add option to disable javascript.options.strict for debug builds

Categories

(Core :: DOM: Core & HTML, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: johnjbarton, Assigned: johnjbarton)

Details

(Whiteboard: [firebug-p1])

Attachments

(1 file)

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.
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.
Whiteboard: [firebug-p1]
ccing some UI-like folks for their feedback, since this primarily affects them.
(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.
(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.
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.
(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.
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.
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.
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.
(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....
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.
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.
(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.
(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?
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
Attachment #393068 - Flags: review?(bzbarsky)
Comment on attachment 393068 [details] [diff] [review]
508562-lessStrict.diff 

Looks ok, but jst should give this a once-over
Attachment #393068 - Flags: superreview?(jst)
Attachment #393068 - Flags: review?(bzbarsky)
Attachment #393068 - Flags: review+
Attachment #393068 - Flags: superreview?(jst) → superreview+
Assignee: nobody → johnjbarton
Pushed http://hg.mozilla.org/mozilla-central/rev/c11e04dd702f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Thanks! I'll check it in 2 weeks when I return.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: