423443 - JS component loader doesn't SetThreadStackLimit

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Mike Shaver (:shaver emeritus)]

Bug 419661 shows a pathological object graph being toSource()d and blowing the C stack due to recursion under MarkSharpObjects.  We should make that be a too-much-recursion exception instead, IMO.

Blocking here for what should be an easy fix to a content-triggerable crasher.

Comment 1

[Brian Crowder]

Looks like Igor added just such a check on 01-FEB-2008, perhaps it is not functioning properly?

Comment 2

[Mike Shaver (:shaver emeritus)]

That's what I get for assuming!  Yeah, it looks like it's not preventing the crash from bug 419661 at the least.

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

TM --> mozilla1.9, this won't block beta 5. If you disagree, please reset the TM to beta5 and explain why it needs to block beta.

Comment 4

[Brian Crowder]

This bug needs a good testcase.  I've modified the summary to describe the issue a little better.  Pushing off to a later milestone is fine, though; I doubt this is a regression -- if anything, we probably do better on this in the trunk than we do on branch.

Comment 5

[Damon Sicore (:damons)]

Who owns this?

Comment 6

[Brian Crowder]

Since this isn't a regression and is just one of a zillion possible DOS attacks, I don't think it necessarily needs and owner or should block 1.9; if shaver disagrees, I'll accept it.  I worked for a short while this morning on creating a test-case, but all the testcases I tried yield appropriate results (a catchable "too much recursion" exception)

js> let graph; for (let i = 0; i < 600; i++) graph = { next: graph }
[object Object]
js> try { uneval(graph) } catch(e) { print("caught: " + e) }
caught: InternalError: too much recursion

Comment 7

[Damon Sicore (:damons)]

-'ing based on comment #6.  Shaver, please feel free to re-nom/block if needed.

Comment 8

[Mike Shaver (:shaver emeritus)]

All good here.  If we can get vlad in the debugger with a lead pipe hitting the case from bug 419661, we can probably tease it out whenever.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

The testcase URL I added to bug 419661 is reliable. It doesn't crash Firefox at anytime but also throws an exception due to max recursion in the Error Console. If I could help in any way, please give instructions.

Comment 10

[Mike Shaver (:shaver emeritus)]

Throwing the exception is the right behaviour, and this bug is about that exception not being thrown before we run out of C stack -- your report indicates that this check is functioning as we want, confusingly!  (Which means that your tescase probably isn't a testcase for 419661 either, since that bug is about a stack-overflow crash and yours doesn't crash.)

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

It will crash Firefox but not at anytime I do these STR. You can see the crash here: bp-ecfb38cf-f878-11dc-ae08-001a4bd46e84

I haven't run enough tests to be able to say why it crashes sometimes or shows the exception like it should.

Comment 12

[Mike Shaver (:shaver emeritus)]

So, I have this in a debugger now, on a context that doesn't look like it's been through the good parts of ContextCallback.

(gdb) p cx->stackLimit
p cx->$5 = 0
(gdb) p cx->scriptStackQuota
$6 = 33554157
(gdb) p/x $
$7 = 0x1fffeed

(http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpcjsruntime.cpp#234)

Similarly, it hasn't apparently been through either of
http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/src/xpccomponents.cpp#3369
or
http://mxr.mozilla.org/mozilla/source/dom/src/base/nsJSEnvironment.cpp#1143

(gdb) p/x cx->operationLimit
$8 = 0x7fffffff
(gdb) p cx->operationCallback
$9 = (JSOperationCallback) 0
(gdb) p cx->operationCallbackIsSet 
$10 = 0

I wonder where this magical mystery context came from!

(gdb) p cx->stackPool->arenasize
$25 = 256          

Only two callers in the tree who set that stackChunkSize:

mozJSComponentLoader::ReallyInit:
http://mxr.mozilla.org/mozilla/source/js/src/xpconnect/loader/mozJSComponentLoader.cpp#517

and
_newJSDContext
http://mxr.mozilla.org/mozilla/source/js/jsd/jsd_high.c#134

We have a global that's a BackStage pass and has OPTION_XML set, so it looks like this is a context created by the component loader.

I propose that the component loader set the stack limit!

Comment 13

[Mike Shaver (:shaver emeritus)]

(This is the session store case, BTW:

  filename = 0x15495c41 "file:///Users/jacob/cvs/moz/mozilla/dist/MinefieldDebug.app/Contents/MacOS/components/nsSessionStore.js", 
  lineno = 1888, 

)

Comment 14

[Mike Shaver (:shaver emeritus)]

Attachment: untested, but COME ON!

Obviously correct, amirite?  (vlad'll test in a sec, he can trigger this on demand.)

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 311504 [details] [diff] [review]
untested, but COME ON!

Can't we instead find a way to use ContextCallback from xpconnect/src/xpcjsruntime.cpp?

/be

Comment 16

[Mike Shaver (:shaver emeritus)]

Possibly, and I started by looking at how to refactor for that, but I didn't feel up to analyzing where the TLS data gets initialized and making sure that it was in the right state when we got called here.  (ContextCallback is prepared to be called with no TLS, so I presume it can happen.)

Would it be too gross to approach that in a follow-up?  There's error-reporting unification that would be nice too, now that they're all part of one module.

Comment 17

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

That patch seems to stop my crash, but I still get the XPConnect warnings a ton..

(###!!! ASSERTION: XPConnect is being called on a scope without a 'Components' property!
)

Comment 18

[Mike Shaver (:shaver emeritus)]

Extremely unlikely that those assertions are related to the lack of recursion checking.  Can you file a separate bug with a stack trace (incl DumpJSStack() if possible) for that assertion?

Thanks for testing the patch!

Comment 19

[Mike Shaver (:shaver emeritus)]

Comment on attachment 311504 [details] [diff] [review]
untested, but COME ON!

Requesting review, in hopes that the unification can wait until after we ship FF3.  I'll write a test plan before requesting approval -- I need to look at what we can do to cause deep native stack recursion, though clearly one path is to create a ginormous narrow+deep object graph!

Comment 21

[Brendan Eich [:brendan]]

Comment on attachment 311504 [details] [diff] [review]
untested, but COME ON!

Sure!

/be

Comment 22

[Mike Shaver (:shaver emeritus)]

In fact, I've decided that MarkSharpObjects is probably nest for this in the test suite: it doesn't rely on deep script stack in order to trigger deep native stack, meaning that tuning of the script stack quota isn't as likely to mask regressions in this behaviour.

So the test JS component would look like:

o = {};
for (var i = 0; i < 1024 * 1024; i++)
  o = o.next = {};
try {
  uneval(o);
  fail(); // detect improvements in MSO that defeat test!
} catch (e if isTooMuchRecursion(e)) {
  pass();
} catch (e) {
  fail();
}

where a crash is also a fail.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 311504 [details] [diff] [review]
untested, but COME ON!

a1.9b5=beltzner

Comment 24

[Mike Shaver (:shaver emeritus)]

Checking in mozJSComponentLoader.cpp;
/cvsroot/mozilla/js/src/xpconnect/loader/mozJSComponentLoader.cpp,v  <--  mozJSComponentLoader.cpp
new revision: 1.159; previous revision: 1.158
done

Comment 25

[Mike Shaver (:shaver emeritus)]

We're still seeing this crash in some cases, now with the limit set -- I wonder if the limit is too generous?  Need to debug some more, obv.