886850 - Assertion failure: [barrier verifier] Unmarked edge: method, at gc/Verifier.cpp:572 with setObjectMetadataCallback

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision bc569033125a (no options required):


evaluate("\
for ( var time = 0, year = 1969; year >= 0; year-- )\
  time -= TimeInYear(year);\
function TimeInYear( y ) {}\
gczeal(4);\
", { noScriptRval : true });
evaluate("setObjectMetadataCallback();", { noScriptRval : true });

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 3

[Christian Holler (:decoder)]

Brian, this is causing lots of signatures, can you look into this?

Comment 4

[Brian Hackett [Laid off!]]

Can you get a blame cset?  setObjectMetadataCallback doesn't do anything interesting when it isn't passed any arguments.

Comment 5

[Christian Holler (:decoder)]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/be1399f8f973
user:        Brian Hackett
date:        Thu May 30 17:37:22 2013 -0600
summary:     Bug 850026 - Allow metadata objects to be associated with JS objects, and add a hook for attaching metadata to newly created objects, r=luke.

This iteration took 1.042 seconds to run.

Comment 6

[Christian Holler (:decoder)]

Setting needinfo on Brian, although this has probably a different cause just like the other bugs with setObjectMetadataCallback. This generates a lot of signatures though so we need to fix it.

Comment 7

[Brian Hackett [Laid off!]]

Attachment: patch

Calling ReleaseAllJITCode seems to be missing some pre barrier somewhere.  Since this is only ever used by debugging tools it seems fine to just let any in progress incremental GC finish.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 781824 [details] [diff] [review]
patch

I thought about this a little and I'm not sure it's right. The basic problem is that JSScript has pointers to IonScripts and BaselineScripts. Those aren't GC things, but they have trace methods, and those trace methods trace GC things. So we need to be sure that, any time we overwrite one of these fields, we call the trace function.

This happens correctly in Invalidate, but it seems like there are other places where we don't have a barrier. For example, nukeTypes calls FinishInvalidation, and that doesn't seem to have a barrier. Also, there's a weird barrier inside InvalidateActivation that's only on the method_ field; I don't understand why it's there.

Anyway, I don't understand this code well enough to review. Can you look it over, Jan? As long as ReleaseAllJITCode is the only problem, I think the patch is fine. I'm just worried about other cases.

Comment 9

[Jan de Mooij [:jandem]]

Yeah I agree with comment 8; a more targeted fix would be good if possible.

Comment 10

[Andrew McCreight [:mccr8]]

Marking critical per comment 8.

Comment 11

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 12

[Brian Hackett [Laid off!]]

Attachment: patch

Per comment 8, write barriers around ion/baseline scripts are a mess.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 784627 [details] [diff] [review]
patch

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

Nice, thanks!

::: js/src/jsscriptinlines.h
@@ +162,5 @@
>      enclosingScopeOrOriginalFunction_ = fun;
>  }
>  
> +inline void
> +JSScript::setIonScript(js::ion::IonScript *ionScript) {

Style nit: brace on its own line here and below.

Comment 14

[Jan de Mooij [:jandem]]

Wrong bug, sorry.

Comment 15

[Brian Hackett [Laid off!]]

Comment on attachment 784627 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

I don't think that what this bug is fixing is actually exploitable.  We could miss a pre barrier under some extremely difficult to trigger conditions (not easily controllable OOM).

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

No.

Which older supported branches are affected by this flaw?

All Ion branches probably.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They should be easy, but I don't really know.

How likely is this patch to cause regressions; how much testing does it need?

Unlikely.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 784627 [details] [diff] [review]
patch

sec-approval+ for trunk. 

I'm neutral on whether we need this on aurora or beta but you could nominate it for both and check it in there assuming branch approvals are given.

Comment 17

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6292baed5ec3

Comment 18

[Brian Hackett [Laid off!]]

Oops, thanks for reverting that flag bugzilla.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6292baed5ec3

Comment 20

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 21

[Alex Keybl [:akeybl]]

Let's take this up to Aurora, since its necessity on Beta is debatable.

Comment 22

[Alex Keybl [:akeybl]]

Never got uplift to Aurora, so untracking completely.

Comment 23

[Daniel Veditz [:dveditz]]

At some point, either now in 25 or next time in 26, we need this on ESR24. If we fix it now we also need it on beta-25.

Comment 24

[Al Billings [:abillings - ex-MoCo]]

Moving the ESR24 tracking to the Firefox 26 release since Alex won't fixed this for Firefox 25.

Comment 25

[Cameron Kaiser [:spectre]]

I'm getting this compilation failure (narrowed down to this bug) in both gcc 4.6.4 and 4.8.2:

/Volumes/BruceDeuce/src/mozilla-26b/js/src/jsscriptinlines.h: In member function 'void JSScript::setBaselineScript(js::jit::BaselineScript*)':
/Volumes/BruceDeuce/src/mozilla-26b/js/src/jsscriptinlines.h:128:9: error: incomplete type 'js::jit::BaselineScript' used in nested name specifier
         js::jit::BaselineScript::writeBarrierPre(tenuredZone(), baseline);
         ^

Comment 26

[Al Billings [:abillings - ex-MoCo]]

This is still not fixed in ESR24 two weeks before release. Can we change this, please?

Comment 27

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Al Billings [:abillings] from comment #26)
> This is still not fixed in ESR24 two weeks before release. Can we change
> this, please?

Setting needinfo from bhackett.

Comment 28

[Brian Hackett [Laid off!]]

Comment on attachment 784627 [details] [diff] [review]
patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
Fix Landed on Version:
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 29

[Brian Hackett [Laid off!]]

This was never my bug.

Comment 30

[Jan de Mooij [:jandem]]

Clearing needinfo. Brian requested approval.

Comment 31

[Al Billings [:abillings - ex-MoCo]]

This is approved by Alex. Can someone check it in?

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/4ef90f0eecf2

Can we land the test now?

Comment 33

[Al Billings [:abillings - ex-MoCo]]

Since this is sec-high, the test shouldn't land until six weeks after this release (or in seven weeks) so avoid pwning ourselves.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

This hit bustage that I didn't know how to resolve and couldn't get help from #jsapi on. Backed out.
https://hg.mozilla.org/releases/mozilla-esr24/rev/df993b197a7e

https://tbpl.mozilla.org/php/getParsedLog.php?id=31333197&tree=Mozilla-Esr24

Comment 35

[Cameron Kaiser [:spectre]]

This is the same problem I reported in comment 25. If this helps analysis, for certain files the way to get it to compile was to add #include "jsscript.h" and remove #include "jit/BaselineJIT.h" but obviously this breaks down for compiling stuff in jit/. I eventually had to backout the patch.

Comment 36

[Brian Hackett [Laid off!]]

The problem looks like Assembler-x86.h unnecessarily including jsscriptinlines.h, creating an include cycle.

https://hg.mozilla.org/releases/mozilla-esr24/rev/a8653b21c44b