Christian Holler (:decoder) Reporter
	Description • 10 years ago

The following testcase crashes on mozilla-central revision 87c8f870e2b9 (run with --fuzzing-safe --ion-compile-try-catch --ion-eager):


this.__defineGetter__("x",
  function() {
    return this;
  }
);
function callback(obj) {}
setObjectMetadataCallback(callback);
evaluate("\
var { ArrayType, StructType, uint32 } = TypedObject;\
  var L = 1024;\
  var Matrix = uint32.array(L, 2);\
  var matrix = new Matrix();\
  for (var i = 0; i < L; i++)\
    matrix[i][0] = x;\
", { compileAndGo : true });

	Christian Holler (:decoder) Reporter
	Comment 2 • 10 years ago

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
js::jit::JitFrameIterator::operator++ (this=0x7fffffffa708) at jit/IonFrames.cpp:316
316         frameSize_ = prevFrameLocalSize();
#0  js::jit::JitFrameIterator::operator++ (this=0x7fffffffa708) at jit/IonFrames.cpp:316
#1  0x0000000000999add in js::FrameIter::settleOnActivation (this=0x7fffffffa6b0) at vm/Stack.cpp:576
#2  0x000000000048eeda in ScriptFrameIter (this=0x7fffffffa6b0, savedOption=js::FrameIter::STOP_AT_SAVED, cx=0x1840c60) at vm/Stack.h:1734
#3  NonBuiltinScriptFrameIter (opt=js::FrameIter::STOP_AT_SAVED, cx=0x1840c60, this=0x7fffffffa6b0) at vm/Stack.h:1826
#4  ShellObjectMetadataCallback (cx=0x1840c60, pmetadata=0x7fffffffabd0) at builtin/TestingFunctions.cpp:1146
#5  0x000000000087e8d0 in callObjectMetadataCallback (obj=0x7fffffffabd0, cx=0x1840c60, this=<optimized out>) at jscompartment.h:346
#6  NewObjectMetadata (pmetadata=0x7fffffffabd0, cxArg=0x1840c60) at jsobjinlines.h:1066
#7  NewObject (cx=0x1840c60, type_=<optimized out>, parent=0x7ffff5642060, kind=js::gc::FINALIZE_OBJECT8_BACKGROUND, newKind=js::GenericObject) at jsobj.cpp:1253
rax     0x8     8
rbx     0xffffa708      140737488332552
rcx     0x0     0
rdx     0x0     0
rsi     0xffffa6f8      140737488332536
rdi     0xffffa708      140737488332552
rbp     0xffffa4e0      140737488332000
rsp     0xffffa4c0      140737488331968
r8      0x0     0
r9      0x1     1
r10     0x0     -1970324836974592
r11     0x4000000       67108864
r12     0xffffa708      140737488332552
r13     0x0     0
r14     0x1840c60       25431136
r15     0xffffa6d8      140737488332504
rip     0x652704 <js::jit::JitFrameIterator::operator++()+36>
=> 0x652704 <js::jit::JitFrameIterator::operator++()+36>:       mov    0x8(%r13),%r12

	Christian Holler (:decoder) Reporter
	Comment 3 • 10 years ago

JSBugMon: Bisection requested, result:
=== Tinderbox Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20140424020245" and the hash "079f4f0ed6a6".
The "bad" changeset has the timestamp "20140424022242" and the hash "462059b115ec".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=079f4f0ed6a6&tochange=462059b115ec

	Christian Holler (:decoder) Reporter
	Comment 4 • 10 years ago

Still hitting this all the time, needinfo on Brian now since it involves setObjectMetadataCallback.

Brian, any idea who could look at this?

The bisect looks bogus, I'm going to try a compiled bisect.

	Christian Holler (:decoder) Reporter
	Comment 5 • 10 years ago

JSBugMon: Bisection requested, failed due to error (try manually).

	Christian Holler (:decoder) Reporter
	Comment 8 • 10 years ago

Needinfo on Brian and Jandem to figure out who might be able to fix this. It still triggers all the time.

	Jan de Mooij [:jandem]
	Comment 9 • 10 years ago

The second stack trace (see attachment) points to the RNewObject recover instruction. The shell's object metadata callback then tries to get a stack trace but this fails because we're in a bailout.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 10 • 10 years ago

I was able to reproduce this issue with both optimized x86 and optimized x64 builds on Linux.  No assertion / segv on debug builds.

Even if this might hurt correctness in a few cases (when Ion guesses are incorrect), I think preventing such callback to be executed on RNewObject* is likely to be a good way to go, as RNewObject* are generated when Ion notices that the object is unused / not-escaped.

The other options are to enable stack iterations on a bailing stack frames, or to prevent any form of stack iterations while we are in a bailout, in which case the correctness issue would be restricted to a missing stack trace instead of a missing callback call.

	Brian Hackett [Laid off!]
	Comment 12 • 10 years ago

Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

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

::: js/src/jit/Recover.cpp
@@ +850,5 @@
>      RootedObject templateObject(cx, &iter.read().toObject());
>      RootedValue result(cx);
>      JSObject *resultObject = nullptr;
>  
> +    // Use AutoEnterAnalysis to prohibit any stack iteration during a bailout.

AutoEnterAnalysis doesn't have any direct effect on stack iteration, so how about:

// Use AutoEnterAnalysis to avoid invoking the object metadata callback while bailing out, which could try to walk the stack.

	Christian Holler (:decoder) Reporter
	Comment 13 • 10 years ago

Marked s-s per nbp's comment on IRC.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 14 • 10 years ago

The setObjectMetadataCallback is only used in Testing functions?  What is this function supposed to mirror?  Dev-tools introspection? GC?

Do we have anything similar which can be impacted if we do not backport this change?

	Brian Hackett [Laid off!]
	Comment 15 • 10 years ago

The object metadata callback is only used for devtools stuff, and not during normal browsing.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 16 • 10 years ago

(In reply to Brian Hackett (:bhackett) from comment #15)
> The object metadata callback is only used for devtools stuff, and not during
> normal browsing.

Thanks, this means that we should backport this patch to 32.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 17 • 10 years ago

If This is only used by dev-tools and only exposed to privileged JS, should we keep the s-s flag?  Should I asked for sec-approval and strip the comments out of the patch?

	Al Billings [:abillings - ex-MoCo]
	Comment 18 • 10 years ago

I'm not sure that we need to keep this hidden.

What is the worst case scenario here for impact?

	Nicolas B. Pierron [:nbp] Assignee
	Comment 19 • 10 years ago

I guess the worse is that we might crash when a developer requests to find the stack traces of all objects allocations, and during a bailout we need to reconstruct an empty object or a typed object.

	Daniel Veditz [:dveditz]
	Comment 20 • 10 years ago

This is sec-high or -critical lowered to sec-moderate because it's not accessible to the web. The comments in the patch are fine.

	Nicolas B. Pierron [:nbp] Assignee
	Comment 21 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/9fe1e613cf21

	Nicolas B. Pierron [:nbp] Assignee
	Comment 22 • 10 years ago

Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

Approval Request Comment
[Feature/regressing bug #]:
Bug 1004527, Bug 1005532 

[User impact if declined]:
Crash while inspecting allocations during dev-tools.

[Describe test coverage new/current, TBPL]:
https://treeherder.allizom.org/ui/#/jobs?repo=try&revision=d5fbd9f5a8d8
(running on inbound atm)

[Risks and why]:
This patch prevent calling the function which might crash. Low risk.

[String/UUID change made/needed]:
None

	Ed Morley [:emorley]
	Comment 23 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/9fe1e613cf21

	Christian Holler (:decoder) Reporter
	Comment 24 • 10 years ago

JSBugMon: This bug has been automatically verified fixed.

	Sylvestre Ledru [:Sylvestre]
	Comment 25 • 10 years ago

Comment on attachment 8453150 [details] [diff] [review]
Prevent stack iterations while recovering allocations.

Checked on IRC with Nicolas. Also accept for beta.

	Ryan VanderMeulen [:RyanVM]
	Comment 26 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/802588f871fc
https://hg.mozilla.org/releases/mozilla-beta/rev/b801d912ea0e

	Ryan VanderMeulen [:RyanVM]
	Comment 27 • 10 years ago

Follow-up fix for the test to early-quit if TypedObject isn't enabled.

https://hg.mozilla.org/releases/mozilla-aurora/rev/40218ed779cb
https://hg.mozilla.org/releases/mozilla-beta/rev/794d229b5125

I'll push it to inbound as a ride-along next time I'm pushing there.

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

Pushed to m-c as a ride-along with the most recent set of merges.
https://hg.mozilla.org/mozilla-central/rev/e95658da5a3d

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 10 years ago

https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/b801d912ea0e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/794d229b5125