Last Comment Bug 695908 - Use bool in js/src/frontend rather than JSBool
: Use bool in js/src/frontend rather than JSBool
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86 Mac OS X
: -- normal (vote)
: mozilla16
Assigned To: Jason Orendorff [:jorendorff]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-19 16:14 PDT by Jason Orendorff [:jorendorff]
Modified: 2012-07-07 12:03 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Part 1 - Boolean literals, v1 (180.02 KB, patch)
2011-10-19 16:16 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Part 1 - Boolean literals, v2 (99.20 KB, patch)
2012-07-02 13:14 PDT, Jason Orendorff [:jorendorff]
ejpbruel: review+
Details | Diff | Splinter Review
Part 2 - Switch from JSBool to bool (39.82 KB, patch)
2012-07-02 13:16 PDT, Jason Orendorff [:jorendorff]
ejpbruel: review+
Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2011-10-19 16:14:25 PDT

    
Comment 1 Jason Orendorff [:jorendorff] 2011-10-19 16:16:01 PDT
Created attachment 568247 [details] [diff] [review]
Part 1 - Boolean literals, v1
Comment 2 Jason Orendorff [:jorendorff] 2012-07-02 13:14:54 PDT
Created attachment 638469 [details] [diff] [review]
Part 1 - Boolean literals, v2

Simple search and replace.
Comment 3 Jason Orendorff [:jorendorff] 2012-07-02 13:16:33 PDT
Created attachment 638470 [details] [diff] [review]
Part 2 - Switch from JSBool to bool

Simple search and replace again, separate only because types can be trickier in C++.

Still, I looked at each case at least briefly and found no trickiness.
Comment 4 Eddy Bruel [:ejpbruel] 2012-07-03 10:29:14 PDT
Comment on attachment 638469 [details] [diff] [review]
Part 1 - Boolean literals, v2

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

Looks good to me. r+ provided it builds.
Comment 5 Eddy Bruel [:ejpbruel] 2012-07-03 10:29:54 PDT
Comment on attachment 638470 [details] [diff] [review]
Part 2 - Switch from JSBool to bool

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

Looks fine to me as well.
Comment 6 :Ms2ger (⌚ UTC+1/+2) 2012-07-03 11:56:46 PDT
You know jsbool.h isn't about JSBool, right?
Comment 7 Jason Orendorff [:jorendorff] 2012-07-03 15:34:04 PDT
Ms2ger: Yeah, I'm aware of it. What happened was, during my search-and-replace, I came across that line, and it seemed very unlikely that that file actually needed that #include. Sure enough it compiles fine without it.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b23429ce0d81
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e28b1f771c1
Comment 9 :Ehsan Akhgari 2012-07-04 16:53:34 PDT
This bug, or something which was merged to mozilla-central in the same push, caused a permanent orange on Linux32 mochitest-chrome for builds without frame pointers (example log: <https://tbpl.mozilla.org/php/getParsedLog.php?id=13227357&tree=Profiling&full=1>).  This is important because framepointers are not enabled on Aurora, so this permaorange will appear there on the next uplift.  I backed out this patch as part of the rest of the js and xpconnect patches in the same merge push.  Please test this patch locally (and push to the try server if needed by taking out the --enable-profiling command from the Linux32 mozconfig) and reland when you make sure that it's not the cause behind the perma-orange.  I apologize in advance for the inconvenience in case this patch is not at fault.

Backout changeset:
https://hg.mozilla.org/mozilla-central/rev/77407e03e22f
https://hg.mozilla.org/mozilla-central/rev/de50598e2329
Comment 10 :Ehsan Akhgari 2012-07-04 17:17:43 PDT
In order to test whether this patch is at fault, push it to try with --enable-profiling removed from browser/config/mozconfigs/linux32/nightly, and if you get a green Linux Moth run, you're good to go!

And in turn you can reproduce the crash on try with your patch, I'd suggest you keep the possibility of having hit a compiler bug in mind.  :-)
Comment 11 :Ehsan Akhgari 2012-07-04 19:23:05 PDT
Also, note that these backouts indeed made the Linux32 Moth runs on the profiling branch green again, so we can be fairly certain that one of the backed out patches was at fault.
Comment 12 Jason Orendorff [:jorendorff] 2012-07-06 07:59:34 PDT
Ehsan: great work, and thanks for telling me how to proceed.

Green Linux32 Moth runs with --enable-profiling removed:
  https://tbpl.mozilla.org/?tree=Try&rev=5c33be0ba83d

Relanded, therefore:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/936e436201e0
  https://hg.mozilla.org/integration/mozilla-inbound/rev/8361bdf249cb

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