Closed Bug 1102521 Opened 10 years ago Closed 9 years ago

Sort out JSAPI state for CPOW calls

Categories

(Core :: IPC, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s ? ---

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(2 files)

Right now everything just uses AutoSafeJSContext, which isn't right, and leads to crashes like bug 1101769. This problem is compounded by the fact that re-entrant IPC messages don't use AutoNoJSAPI, so things _usually_ don't crash, but might just do the wrong thing [1].

[1] http://pastebin.mozilla.org/7419022
We definitely need to fix this before shipping e10s.
tracking-e10s: --- → ?
I'm not sure if this is right - bent says it might break plugins. Thoughts, Bill?
Attachment #8526379 - Flags: feedback?(wmccloskey)
Comment on attachment 8526379 [details] [diff] [review]
Part 1 - Stop inheriting JSAPI state when re-entering a process (Strawman). v1

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

::: ipc/glue/MessageChannel.cpp
@@ +967,5 @@
>  bool
>  MessageChannel::ProcessPendingRequest(const Message &aUrgent)
>  {
>      AssertWorkerThread();
> +    AutoNoJSAPI nojsapi;

Could you say more about what this does and what you want to achieve? According to the comment for AutoNoJSAPI, this object will be overridden by any AutoJSAPI (and presumably AutoEntryScript) objects pushed on top of it. If that's true, I don't see how this could break anything. Both the message manager and nsJSNPRuntime use AutoEntryScript or AutoJSAPI when they run JS.

So based on what I know, I'd be in favor of expanding this to cover all the places where we dispatch messages. That would be in MessageChannel::DispatchMessage. ProcessPendingRequest is only called when we're waiting for a sync reply and we decide to process an incoming message while we're waiting. That's kind of a corner case.
Comment on attachment 8526380 [details] [diff] [review]
Part 2 - Use AutoEntryScript/AutoJSAPI for CPOW answers. v1

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

::: js/ipc/WrapperAnswer.cpp
@@ +68,5 @@
>  WrapperAnswer::RecvPreventExtensions(const ObjectId &objId, ReturnStatus *rs,
>                                       bool *succeeded)
>  {
> +    AutoJSAPI jsapi;
> +    if (NS_WARN_IF(!jsapi.Init(scopeForTargetObjects())))

It looks like AutoJSAPI::Init already does NS_WARN_IF on its error paths, so doing it here seems redundant. Also, it only appears to fail if the underlying global JSObject* is null. I don't think that can happen here since we're passing in the JSObject* itself. Could we just assert instead?

@@ +284,5 @@
>  WrapperAnswer::RecvGet(const ObjectId &objId, const ObjectVariant &receiverVar,
>                         const JSIDVariant &idVar, ReturnStatus *rs, JSVariant *result)
>  {
> +    // We may run scripted getters.
> +    AutoEntryScript aes(xpc::NativeGlobal(scopeForTargetObjects()));

Why do we have to call Init for AutoJSAPI but not for AutoEntryScript?
Attachment #8526380 - Flags: review?(wmccloskey) → review+
> It looks like AutoJSAPI::Init already does NS_WARN_IF on its error paths, so
> doing it here seems redundant.

The file/line number of the caller are much more useful in a log than the file/line within AutoJSAPI.

> Also, it only appears to fail if the
> underlying global JSObject* is null. I don't think that can happen here
> since we're passing in the JSObject* itself. Could we just assert instead?

We could, I guess, but I'd rather that the API convention remain relatively consistent, and robust in case we introduce a new failure mode later on.

> Why do we have to call Init for AutoJSAPI but not for AutoEntryScript?

Because AutoEntryScript was never updated to be fallible - see bug 1094953#c6.

I'm going to split off Part 1 into a new bug, since it's just for robustness and isn't needed to fix the crash here.
Blocks: 1103324
https://hg.mozilla.org/mozilla-central/rev/f456607aaa4c
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8526379 [details] [diff] [review]
Part 1 - Stop inheriting JSAPI state when re-entering a process (Strawman). v1

This got turned into bug 1103324.
Attachment #8526379 - Flags: feedback?(wmccloskey)
You need to log in before you can comment on or make changes to this bug.