667915 - Assertion failure: pc_ >= script->code && pc_ < script->code + script->length | Assertion failure: compartment mismatched | Crash [@ moz_free | NS_Free_P nsMemory::Free(void*) XPCJSStackFrame::~XPCJSStackFrame() ]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

1. http://www.autoteile-immler.com/THULE+973+BackPac+Fahrradtr%e4ger+inkl.+Montagekit+973-15/a5730277_u161/

2. stop or continue slow script.

3. Crash with multiple signatures or assertions.

Mac OS X 10.5

Assertion failure: pc_ >= script->code && pc_ < script->code + script->length, at /work/mozilla/builds/nightly/mozilla/js/src/vm/Stack.cpp:978

Linux 64 bit

Assertion failure: compartment mismatched

Windows XP and 7

moz_free | NS_Free_P nsMemory::Free(void*) XPCJSStackFrame::~XPCJSStackFrame() XPCJSStackFrame::`vector deleting destructor'(unsigned int) + 0xe XPCJSStackFrame::Release()

Comment 1

[Bob Clary [:bc] (inactive)]

stack for Mac: https://bugzilla.mozilla.org/attachment.cgi?id=542476

Comment 2

[Luke Wagner [:luke]]

So check this out: the code chews up the whole stack (causing an exception to be thrown), when its time to report the exception, we try to push a dummy frame from AutoEnterCompartment, THAT also hits the end of the stack, tries to throw, but now the exception's compartment doesn't match the cx's, hence the assert.

Comment 3

[Luke Wagner [:luke]]

Attachment: Fix AutoCompartment::enter

This fixes the compartment assert (which I can repro on linux64 by disabling all jits): AutoCompartment::enter switches the compartment before pushing the dummy frame (hence the mismatch when the CompartmentChecker asserts that GetGlobalForScopeChain (fp->scopeChain() in this case) is in cx->compartment).  I'm a bit confused why it was written this way in the first place; perhaps there is some hidden constraint here?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 542601 [details] [diff] [review]
Fix AutoCompartment::enter

My only thought would be a fear that we'd have to read cx->compartment in order to allocate stack space for the dummy frame, but that doesn't appear to be the case, so r=mrbkap.

Comment 5

[Luke Wagner [:luke]]

Attachment: Fix FixupArity

FixupArity calls getFixupFrame which reports despite a trashed regs.pc.

With these patches, I don't get any asserts.  But this does expose a problem I've been a'fearin', namely, that content can use up the stack and OOM unsuspecting chrome JS.  I'm going to think about this.

Comment 6

[Luke Wagner [:luke]]

Attachment: don't let content oom chrome (WIP)

So its actually pretty easy for content to oom chrome.  For example, this page will use up all the stack space such that the slow-script dialog will fail, eventually locking up the entire browser while running at 100% CPU:

  <script>
  function foo() {
    try {
      foo()
    } catch(e) {
      while (true) ;
    }
  </script>

The attached patch fixes this test case and also causes the page in comment 0 to have a normal slow-script dialog.  The approach is to report a smaller stack size to any script that doesn't have system principals.  That way, content ooms early and, when chrome runs, it still has plenty of space.  Still a bit of work to do.

Comment 7

[Luke Wagner [:luke]]

Attachment: don't let content oom chrome

Mostly same patch but with build fixes for windows and a JSAPI test of the new mechanism.  This patch subsumes the other two and includes shell test-cases to trigger the specific asserts in the previous two patches.

All in all, I'm pretty happy to have this patch: its relatively simple and it fixes a long-standing concern since the introduction of the contiguous stack.  Incidentally, the other three browsers all show issues with the testcase above:
 - IE hangs and needs to be force-quit
 - Safari hangs and then shows what is probably supposed to be their slow-script dialog, but its a tiny empty rectangle. Fortunately, their default-close behavior kills the script.
 - Chrome UI remains responsive (process isolation ftw), but the tab is unresponsive and doesn't display the slow-script dialog that normally is shown; it has to be killed by clicking the tab's 'x'.

Blake: could you check out the xpconnect/caps changes and see if that all look sane?
Waldo: can you look at the rest?

Comment 8

[Luke Wagner [:luke]]

Green on try (once "if (getMaxArgs)" is added to regress-350256-02.js).

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 543291 [details] [diff] [review]
don't let content oom chrome

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

::: js/src/vm/Stack.h
@@ +1357,5 @@
>       * LeaveTree requires stack allocation to rebuild the stack. There is no
>       * good way to handle an OOM for these allocations, so this function checks
>       * that OOM cannot occur using the size of the TraceNativeStorage as a
> +     * conservative upper bound. Despite taking a 'cx', this function does not
> +     * report an error if it returns 'false'.

I'd pull the last sentence into its own paragraph; it blends in a bit here.

@@ +1362,2 @@
>       */
> +    inline bool ensureEnoughSpaceToEnterTrace(JSContext *cx, MaybeReportError report);

This method's always called with report == NO_REPORT_ERROR.  Is that for documentation purposes?  I'm not sure I see anything clearly better, but maybe there is something.

Comment 10

[Bob Clary [:bc] (inactive)]

another url: http://www.dualsimtelefone.de/Dual-Sim-Telefone-Anycool-T728-Dualsim-Touchscreen-Handy-MP3-MP4-NEU-OVP/a31177604_u4357_z922d8337-124d-4a11-a0d4-68e54fedfb5a/

let the script continue.

Comment 11

[Luke Wagner [:luke]]

(In reply to comment #10)
Thanks for the test-case.  With the patch, I can repeatedly click 'Continue' and eventually 'Stop' and the page is responsive with no asserts.

Comment 12

[Luke Wagner [:luke]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/28be8df0deb7

Comment 13

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/28be8df0deb7
http://hg.mozilla.org/mozilla-central/rev/0661e7077dce

Comment 14

[IU]

The patch aside, why is firefox even hanging on those pages?  For me, neither chrome 13 nor ie 9 hang on those pages, and opera 11.5 only momentarily pauses on the comment 10 page.  So while this patch helps firefox recover, it would be nice if there was a follow-up to address the root problem.

Comment 15

[Luke Wagner [:luke]]

The root problem is we don't have content/chrome process separation.  Rest assured, work is underway: https://wiki.mozilla.org/Electrolysis.

A secondary root problem is that, if we have an error while running the operation callback, apparently we either (1) lose it or (2) choose not to halt the running script.  That sounds like a nice bug to file, if you'd like.

Comment 16

[IU]

(In reply to comment #15)

Thanks, Luke.

> A secondary root problem is that, if we have an error while running the
> operation callback, apparently we either (1) lose it or (2) choose not to
> halt the running script.  That sounds like a nice bug to file, if you'd like.

Don't understand enough to file that one.  Any takers?

Comment 17

[Luke Wagner [:luke]]

Filed bug 670183.

To be clear, I think the problem fixed by this bug is as "root" of a problem as the other two things; it fixes a class of DOSes.

Comment 19

[Bob Clary [:bc] (inactive)]

Luke, is this really fixed on Beta/Firefox 8? I see a very similar crash (stack overflow) with oodles of 

XPCJSStackFrame::Release()
nsCOMPtr<nsIStackFrame>::~nsCOMPtr<nsIStackFrame>()
XPCJSStackFrame::~XPCJSStackFrame()

on the stack when visiting http://www.in-town.nl/web/ I don't see it in the automation for Aurora/9, or Nightly/10.

Comment 20

[Luke Wagner [:luke]]

That stack trace would almost certainly have been fixed by bug 676934.  (Basically, we increased the scripted stack depth max (to something comparable to other engines) and it looks like XPCJSStackFrame was doing a recursive destructor call for each such frame which apparently is enough to overflow the C stack.  Bug 676934 just bounded the number of XPCJSStackFrames we create.)