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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: johnjbarton, Assigned: johnjbarton)
Details
(Whiteboard: [firebug-p1])
Attachments
(1 file)
2.78 KB,
patch
|
bzbarsky
:
review+
jst
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•15 years ago
|
||
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]
Comment 2•15 years ago
|
||
ccing some UI-like folks for their feedback, since this primarily affects them.
Comment 3•15 years ago
|
||
(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.
Assignee | ||
Comment 4•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 6•15 years ago
|
||
(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.
Assignee | ||
Comment 7•15 years ago
|
||
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.
Assignee | ||
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
(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•15 years ago
|
||
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.
Assignee | ||
Comment 12•15 years ago
|
||
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.
Assignee | ||
Comment 13•15 years ago
|
||
(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.
Assignee | ||
Comment 14•15 years ago
|
||
(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•15 years ago
|
||
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
Assignee | ||
Updated•15 years ago
|
Attachment #393068 -
Flags: review?(bzbarsky)
Comment 16•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #393068 -
Flags: superreview?(jst) → superreview+
Updated•15 years ago
|
Assignee: nobody → johnjbarton
Comment 17•15 years ago
|
||
Pushed http://hg.mozilla.org/mozilla-central/rev/c11e04dd702f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 18•15 years ago
|
||
Thanks! I'll check it in 2 weeks when I return.
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•