1658827 - Returning an uncloneable object in an actor's receiveMessage handler crashes on debug build: Assertion failure: !cx->isExceptionPending() in Interpreter.cpp:488

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect, P3)

Description

[Rob Wu [:robwu]]

In bug 1655624 I added a unit test (https://phabricator.services.mozilla.com/D85644) that can be run as follows:

./mach test toolkit/components/extensions/test/xpcshell/test_ext_runtime_sendMessage.js --log-mach-verbose --verbose --tag=remote-webextensions

Part of the test causes an JSActor's receiveMessage handler to return a non-cloneable object, which would ordinarily trigger a DataCloneError. On debug builds, this unexpectedly causes a crash due to the following assertion:

Assertion failure: !cx->isExceptionPending(), at /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:488

This is unexpected - there shouldn't be a crash on debug builds.

STR (manually):

1. Visit a page with a frame (e.g. example.com + using the devtools to add an iframe)
2. Open the Browser Content Toolbox, and run the following snippet:

{
  let proto = ChromeUtils.import("resource:///actors/ContextMenuChild.jsm").ContextMenuChild.prototype;
  if (!proto._origRM) proto._origRM=proto.receiveMessage;
  proto.receiveMessage = function(message) {
    if (message.name === "ContextMenu:GetFrameTitle") {
      // This is uncloneable
      return this.contentWindow;
    }
    return this._origRM.call(this, message);
  };
}

3. Right-click in the iframe, choose "This Frame > Bookmark This Frame"

Expected:

• DataCloneError logged in console, but no crash.

Actual:

• Crash on debug build.

Comment 1

[Rob Wu [:robwu]]

I'm going to land the second test-only patch from bug 1655624, with a skip-if=debug to work around this bug. When this bug is fixed, the skip-if can be removed.

EDIT: Actually never mind disabling the test. The test file contains many other tests, and I'm not in a rush to land the tests.

Comment 2

[Chris Peterson [:cpeterson]]

P3

Comment 3

[Rob Wu [:robwu]]

This bug allows anything that can control (part of) the response to a JSActor's receiveMessage handler to trigger a crash on debug builds.

On Android, extensions run in the main process, so this bug would allow extensions to crash the whole browser on debug builds.

Nika, is this something that can be fixed easily? Should the S3 rating be adjusted?

Comment 4

[Nika Layzell [:nika] (ni? for response)]

This exception appears (from a quick guess, haven't run the code) to be being caused due to a native method being called, and returning a success code, despite setting an exception on the JSContext*. Based on this being due to the data clone error being in a receiveMessage response, I am guessing this is being caused by the query handler here: https://searchfox.org/mozilla-central/rev/358cef5d1a87172f23b15e1a705d6f278db4cdad/dom/ipc/jsactor/JSActor.cpp#422-423

Specifically, I think that StructuredCloneData::Write is causing an exception to be set on the JSContext which isn't being cleared when the code is being exited. Based on a quick scan of other implementations of the query handler this seems to be a somewhat common issue, as I rarely see callers clear exceptions from aCx in error cases.

Fixing this specific case shouldn't be too difficult, but it may also be worthwhile to remove the JSContext* parameter, or change the method to allow it to return false and report an exception (though that's probably rarely what you want to do).

Comment 5

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1658827 - Clear exceptions before returning from JSActor promise handlers,

There must be no pending exceptions on the JSContext when returning from a
native promise handler. We were not successfully clearing exceptions in all
cases in JSActor logic, meaning that query replies with unserializable data
would cause debug-mode crashes.

This patch handles JSActor's code, but doesn't improve the situation for other
native promise handlers which could throw exceptions.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ac5156712d7
Clear exceptions before returning from JSActor promise handlers, r=kmag

Comment 7

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4ac5156712d7