770261 - hoist eval code into builtin/Eval.{h,cpp}

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Luke Wagner [:luke]]

Attachment: 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]]

Attachment: patch

hg add builtin/Eval.h helps things, ya know, build.

Comment 2

[Luke Wagner [:luke]]

I'm not going to lie, I was tempted to accidentally name the file builtin/Evil.{h,cpp}.

Comment 3

[Luke Wagner [:luke]]

Green on try

Comment 4

[Jason Orendorff [:jorendorff]]

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]]

(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]]

https://hg.mozilla.org/mozilla-central/rev/8417741974ba

Comment 7

[(no longer active)]

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

[(no longer active)]

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]]

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

[(no longer active)]

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

[(no longer active)]

(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]]

(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

[(no longer active)]

(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.  :-)