Last Comment Bug 770261 - hoist eval code into builtin/Eval.{h,cpp}
: hoist eval code into builtin/Eval.{h,cpp}
Status: RESOLVED FIXED
[js:t]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: Luke Wagner [:luke]
:
Mentors:
Depends on:
Blocks: 767750
  Show dependency treegraph
 
Reported: 2012-07-02 11:03 PDT by Luke Wagner [:luke]
Modified: 2012-07-06 11:57 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (217.13 KB, patch)
2012-07-02 11:03 PDT, Luke Wagner [:luke]
no flags Details | Diff | Splinter Review
patch (218.82 KB, patch)
2012-07-02 11:27 PDT, Luke Wagner [:luke]
jorendorff: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2012-07-02 11:03:49 PDT
Created attachment 638418 [details] [diff] [review]
patch

Note: I used hg copy jsobj.cpp builtin/Eval.cpp (and then removed everything but the eval code) to preserve the history.
Comment 1 Luke Wagner [:luke] 2012-07-02 11:27:46 PDT
Created attachment 638424 [details] [diff] [review]
patch

hg add builtin/Eval.h helps things, ya know, build.
Comment 2 Luke Wagner [:luke] 2012-07-02 18:14:57 PDT
I'm not going to lie, I was tempted to accidentally name the file builtin/Evil.{h,cpp}.
Comment 3 Luke Wagner [:luke] 2012-07-03 10:31:34 PDT
Green on try
Comment 4 Jason Orendorff [:jorendorff] 2012-07-03 18:38:33 PDT
Comment on attachment 638424 [details] [diff] [review]
patch

In builtin/Eval.h:
>+// 'call' should be for the eval/Function native invocation.
>+extern JSPrincipals *
>+PrincipalsForCompiledCode(const CallReceiver &call, JSContext *cx);

This comment could be better.

r=me and thank you!
Comment 5 Luke Wagner [:luke] 2012-07-03 19:05:31 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #4)
Indeed, fixed.

https://hg.mozilla.org/integration/mozilla-inbound/rev/8417741974ba
Comment 6 Ryan VanderMeulen [:RyanVM] 2012-07-04 06:39:51 PDT
https://hg.mozilla.org/mozilla-central/rev/8417741974ba
Comment 7 :Ehsan Akhgari 2012-07-04 16:52:12 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/6517c5178fa9
Comment 8 :Ehsan Akhgari 2012-07-04 17:17:56 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 9 Luke Wagner [:luke] 2012-07-04 19:08:40 PDT
I was able to repro the orange in question on the cset before my patch on try:
  https://tbpl.mozilla.org/?tree=Try&rev=170f2692df66
so I relanded (this time with a pre-emptive clobber):
  https://hg.mozilla.org/mozilla-central/rev/0690da7a7b86
Comment 10 :Ehsan Akhgari 2012-07-04 19:23:14 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 11 :Ehsan Akhgari 2012-07-04 21:55:52 PDT
(In reply to comment #9)
> I was able to repro the orange in question on the cset before my patch on try:
>   https://tbpl.mozilla.org/?tree=Try&rev=170f2692df66
> so I relanded (this time with a pre-emptive clobber):
>   https://hg.mozilla.org/mozilla-central/rev/0690da7a7b86

Can you please mention this in the respective bugs?
Comment 12 Luke Wagner [:luke] 2012-07-04 22:30:49 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #11)
I'm happy to help, but, since this was the last cset in the backout range, I don't think I have any new information to provide; it still might be any one of the other patches.  Maybe I'm missing your point?
Comment 13 :Ehsan Akhgari 2012-07-06 11:57:01 PDT
(In reply to comment #12)
> (In reply to Ehsan Akhgari [:ehsan] from comment #11)
> I'm happy to help, but, since this was the last cset in the backout range, I
> don't think I have any new information to provide; it still might be any one of
> the other patches.  Maybe I'm missing your point?

Oh, no, it was me who was missing your point.  Thanks for the clarification.  :-)

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