Closed
Bug 732665
Opened 12 years ago
Closed 11 years ago
Chrome scripts should have a higher stack (recursion) limit than content scripts
Categories
(Core :: XPConnect, enhancement)
Core
XPConnect
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: jruderman, Assigned: bholley)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
21.99 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.63 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
5.35 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
3.27 KB,
patch
|
luke
:
review+
|
Details | Diff | Splinter Review |
Content can probe for the maximum recursion depth and then cause chrome code to run (or spin the event loop). This can make chrome scripts fail at unexpected points.
Reporter | ||
Updated•12 years ago
|
Summary: Chrome scripts should have a higher recursion limit than content scripts → Chrome scripts should have a higher stack (recursion) limit than content scripts
Reporter | ||
Comment 1•12 years ago
|
||
Luke -- billm thinks you might have already fixed this?
Reporter | ||
Comment 2•12 years ago
|
||
(But maybe only for the script stack, not the native stack?)
Comment 3•12 years ago
|
||
That's right, only for the script stack. It would make sense to do it for the native stack. mrbkap was talking about this also.
Assignee | ||
Comment 4•11 years ago
|
||
Luke, is this something that's relatively easy to do? If so, are you the one to work on it?
Flags: needinfo?(luke)
Comment 5•11 years ago
|
||
Probably easy; seems like you'd just change JS_CHECK_RECURSION to use a different limit when cx->runningWithTrustedPrincipals(). This doesn't involve any details of vm/Stack so anyone could do it. More important would be making a good set of mochitests that fail before the patch and are fixed by the patch.
Flags: needinfo?(luke)
Comment 6•11 years ago
|
||
Bobby, could you hack up a fix here? Sounds easy, would knock off a few bugs with one fix!
Assignee: nobody → bobbyholley+bmo
Assignee | ||
Comment 7•11 years ago
|
||
We should also look at fixing bug 813646 here.
Assignee | ||
Comment 8•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=740216d845b6
Assignee | ||
Comment 9•11 years ago
|
||
Looks like this was good for everything bug linux-debug. Pushing again for that config: https://tbpl.mozilla.org/?tree=Try&rev=9bbfa060ab48
Assignee | ||
Comment 10•11 years ago
|
||
Oh darn - I just realized that I wrote a mochitest-chrome test, not a mochitest-plain, and so I need to do mochitest-o to get those test runs (rather than mochitest-4). Redoing this pushes from comment 8 and comment 9 together: https://tbpl.mozilla.org/?tree=Try&rev=29ab1dfc929c
Assignee | ||
Comment 11•11 years ago
|
||
I'm going to get started by flagging for review on the JS bits. We kind of cop out for ion and PJS stuff. We should at least have a good story for ion, but it probably needs to be done with someone with more jit familiarity than I have. The uninlined function call in JS_CHECK_RECURSION is unfortunate. We could fix this either by separating the jsfriendapi JS_CHECK_RECURSION from the internal one, or moving the appropriate stuff into friend fields.
Attachment #791116 -
Flags: review?(luke)
Assignee | ||
Comment 12•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=d1084c016d5b
Comment 13•11 years ago
|
||
Comment on attachment 791116 [details] [diff] [review] Part 1 - Introduce a mechanism for specifying different stack limits for system, trusted, and untrusted code. v1 Review of attachment 791116 [details] [diff] [review]: ----------------------------------------------------------------- Nice work! Could you add a JSAPI test for this? There is already jsapi-tests/testChromeBuffer.cpp that I added to test the JS stack which hopefully you can extend w/o too much trouble (however, see bug 906040). ::: js/src/jit/ParallelFunctions.cpp @@ +133,5 @@ > // flag is set, so we still need to double check. > > uintptr_t realStackLimit; > if (slice->isMainThread()) > + realStackLimit = slice->runtime()->mainThread.nativeStackLimit[StackForTrustedScript]; I think you could continue to use GetNativeStackLimit if you changed GetNativeStackLimit to take a ThreadSafeContext (instead of ExclusiveContext) which is a public base of ForkJoinSlice (which you have here). ::: js/src/jsapi.cpp @@ +2605,3 @@ > > +void > +js::RecomputeStackLimit(JSRuntime *rt, StackKind kind) This is declared in Runtime.h, can you put it in Runtime.cpp? @@ +2627,5 @@ > > // If there's no pending interrupt request set on the runtime's main thread's > // ionStackLimit, then update it so that it reflects the new nativeStacklimit. > + // > + // Note that, for now, we use the untrusted limit for ion. This comment made me worry that if trusted code accidentally entered Ion that it would get an error, but this is not the case because ionStackLimit is a conservative bound and, when the check fails in jit code, we fall back to C++ code which does a real JS_CHECK_RECURSION. Could you extend the comment to explain this. ::: js/src/jspubtd.h @@ +273,5 @@ > +{ > + StackForSystemCode, > + StackForTrustedScript, > + StackForUntrustedScript, > + StackKindCount Could you add short comments after each enumerator? ::: js/src/vm/Runtime.cpp @@ +461,5 @@ > JS_ASSERT(oldCount > 0); > > #ifdef JS_THREADSAFE > js::TlsPerThreadData.set(NULL); > + PodArrayZero(mainThread.nativeStackLimit); I don't understand the need for this if the JSRuntime is being destroyed.
Attachment #791116 -
Flags: review?(luke) → review+
Assignee | ||
Comment 14•11 years ago
|
||
I already have a mochitest for this stuff, I just haven't posted it yet. Addressed all other feedback, and pushed to try to see if that weird macosx64 orange went away: https://tbpl.mozilla.org/?tree=Try&rev=ae35e464ebdd
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #14) > I already have a mochitest for this stuff, I just haven't posted it yet. > Addressed all other feedback, and pushed to try to see if that weird > macosx64 orange went away: > > https://tbpl.mozilla.org/?tree=Try&rev=ae35e464ebdd Ok, this is green modulo some slow infinite recursion that times out with the new mac stack depth. I'm going to disable a few tests and get this stuff reviewed and landed.
Assignee | ||
Comment 16•11 years ago
|
||
The only change in behavior here is that we give the osx check higher precedence than the DEBUG check, meaning that we end up with 7MB stacks on osx debug rather than 4MB stacks. But this seems strictly better given the logic involved here.
Attachment #794876 -
Flags: review?(luke)
Assignee | ||
Comment 17•11 years ago
|
||
Attachment #794877 -
Flags: review?(luke)
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #794878 -
Flags: review?(luke)
Assignee | ||
Comment 19•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=69305ea18931
Comment 20•11 years ago
|
||
Comment on attachment 794876 [details] [diff] [review] Part 2 - Clean up and clarify the stack sizing logic. v1 Review of attachment 794876 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/xpconnect/src/XPCJSRuntime.cpp @@ +2910,5 @@ > +#define DEFAULT_STACK_QUOTA (128 * sizeof(size_t) * 1024) > + > +// Set stack sizes for different configurations. It's probably not great for > +// the web to base this decision primarily on the default stack size that the > +// underlying platform makes available, but that seems to be what we do. :-( The column of this comment and the above one looks kinda wrong. You make the call but just pointing it out. @@ +2914,5 @@ > +// underlying platform makes available, but that seems to be what we do. :-( > + > +#if defined(XP_MACOSX) || defined(DARWIN) > + // MacOS has a gargantuan default stack size of 8MB. Go wild with 7MB. > +#define STACK_QUOTA (7 * 1024 * 1024) It seems like this could be done with plain old variables instead of #defines. @@ +2923,4 @@ > #elif defined(XP_WIN) > + // 1MB is the default stack size on Windows, so the default 1MB stack quota > + // we'd get on win32 is slightly too large. Use 900k instead. > +#define STACK_QUOTA (runtime, 900 * 1024) This seems like a bug, given the usage of STACK_QUOTA below.
Attachment #794876 -
Flags: review?(luke) → review+
Comment 21•11 years ago
|
||
Comment on attachment 794877 [details] [diff] [review] Part 3 - Use separate stack depths for XPConnect JS. v1 Same comment about vars vs. #defines.
Attachment #794877 -
Flags: review?(luke) → review+
Comment 22•11 years ago
|
||
Comment on attachment 794878 [details] [diff] [review] Part 4 - Tests. v1 Could you add a mochitest that chews up all the stack space in content and then, with all this space chewed up, calls chrome and ensures that chrome runs successfully?
Attachment #794878 -
Flags: review?(luke) → review+
Assignee | ||
Comment 23•11 years ago
|
||
Updated to address all review comments, and did one full try push given that this patch is likely to be pretty orange-prone: https://tbpl.mozilla.org/?tree=Try&rev=d5af3d56492d
Assignee | ||
Comment 24•11 years ago
|
||
Looks green modulo some intermittent failures and an OSX JSReftest failure for whom I apparently used the wrong skip-if syntax in the first patch. remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5d805bda611b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/548651733b5a remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/331225b5d705 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/317317005f47 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/bfe32b93a350
Comment 25•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5d805bda611b https://hg.mozilla.org/mozilla-central/rev/548651733b5a https://hg.mozilla.org/mozilla-central/rev/331225b5d705 https://hg.mozilla.org/mozilla-central/rev/317317005f47 https://hg.mozilla.org/mozilla-central/rev/bfe32b93a350
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•