Get WebIDL callbacks working on workers

RESOLVED FIXED in mozilla26

Status

()

Core
DOM: Workers
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: khuey, Assigned: nsm)

Tracking

unspecified
mozilla26
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

What we need here is the following:

1)  Make CallSetup work on workers.  In the constructor we can just check for main thread
    and if not:
  A) Avoid calling nsContentUtils::GetSafeJSContext: get the JSContext some other way.
  B) Avoid calling mCxPusher.Push.
  C) Avoid doing the security check.

    Not sure whether the destructor needs changes or whether the Pop() is a safe no-op if
    we never did Push().

2)  Assert that CallbackObjectHolderBase::ToXPCOMCallback is only called on the main
    thread.  Similar for ToWebIDLCallback.  Maybe we should just assert if
    CallbackObjectHolder is ever constructed off the main thread?  That might be tough
    for events, though...

3)  Make the this-object wrapping work on workers.  Right now we use WrapCallThisObject,
    which typically calls WrapNativeParent.  This might already be safe on workers, if
    all those objects are nsWrapperCache and WebIDL objects.  If not, we just need to
    make it work.
Created attachment 799618 [details] [diff] [review]
Get callbacks on workers

Adds GetThreadSafeJSContext() to get the right JSContext based on main thread or worker thread.

In the call chain of WrapCallThisObject, tries to first unwrap the object assuming it supports wrappercache, when not on the main thread.
Assignee: nobody → nsm.nikhil
Attachment #799618 - Flags: review?(khuey)
https://tbpl.mozilla.org/?tree=Try&rev=a24bb4b934ea

Try run with this patch applied on top of khuey's event target patches, and a patch to port Promises to workers.
Created attachment 799636 [details] [diff] [review]
Get WebIDL callbacks working on Workers.

Slightly updated patch to fix build bustage on Mac.
Attachment #799636 - Flags: review?(khuey)
Attachment #799618 - Attachment is obsolete: true
Attachment #799618 - Flags: review?(khuey)
Comment on attachment 799636 [details] [diff] [review]
Get WebIDL callbacks working on Workers.

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

Looks pretty good.  r- because I asked you to rework the control flow and I'd like to read that again.

I would be interested to know what bz thinks about the inlining in NativeInterface2JSObjectThrow.. and about the exception handling.

::: content/base/public/nsContentUtils.h
@@ +1533,5 @@
>                                                        nsresult* aRv);
>  
>    static JSContext *GetCurrentJSContext();
>    static JSContext *GetSafeJSContext();
> +  static JSContext *GetThreadSafeJSContext();

Name this ThreadSafeGetSafeJSContext.

All of the alternatives are far worse (especially GetThreadSafeSafeJSContext).

::: dom/bindings/BindingUtils.cpp
@@ +634,5 @@
>                                           const nsIID* aIID,
>                                           bool aAllowNativeWrapper)
>  {
>    nsresult rv;
> +  if (!NS_IsMainThread()) {

I think we should inline this bit regardless of what thread we're on.

::: dom/bindings/CallbackObject.cpp
@@ +56,3 @@
>  
> +    // First, find the real underlying callback.
> +    JSObject* realCallback = js::UncheckedUnwrap(aCallback);

I would prefer that we consolidated more of this, although that probably requires more complicated control flow.

Something like

bool isMainThread = NS_IsMainThread();

if (isMainThread) {
  // microtask
}

JSObject* realCallback ...

if (isMainThread) {
  // Get context one way
} else {
  // Get context the other way
}

xpc_UnmarkGray
mRooted.construct

if (isMainThread) {
  // securitah check
}

construct autocompartment
set mCx, etc

@@ +128,5 @@
> +    // Make sure the JS engine doesn't report exceptions we want to re-throw
> +    if (mExceptionHandling == eRethrowExceptions) {
> +      mSavedJSContextOptions = JS_GetOptions(cx);
> +      JS_SetOptions(cx, mSavedJSContextOptions | JSOPTION_DONT_REPORT_UNCAUGHT);
> +    }

I'm 99% sure we want this on workers too.

::: dom/workers/WorkerPrivate.h
@@ +990,5 @@
>  bool
>  IsCurrentThreadRunningChromeWorker();
>  
> +JSContext *
> +GetCurrentThreadJSContext();

fwiw, I have basically the same thing in one of my patches
Attachment #799636 - Flags: review?(khuey)
Attachment #799636 - Flags: review-
Attachment #799636 - Flags: feedback?(bzbarsky)
Created attachment 799881 [details] [diff] [review]
Get WebIDL callbacks working on Workers

Reworked conditional and other changes as per last review.
Attachment #799636 - Attachment is obsolete: true
Attachment #799636 - Flags: feedback?(bzbarsky)
Attachment #799881 - Flags: review?(khuey)
> I would be interested to know what bz thinks about the inlining in
> NativeInterface2JSObjectThrow

Funny you should ask.

So that code will handle the case when the thisobj thing passed in does not inherit from nsWrapperCache but does QI to it.  I guess that will actually happen in event code....

I'm OK with I guess, except the code being inlined has some problems.  Specifically:

1)  The comment talks about IsProxy() when it means IsDOMBinding().  Fix that in
    XPCConvert too?
2)  "flat" can be declared inside the "if (cache)" block.  In fact inside the
    IsDOMBinding() block.  And maybe call it "obj"?
3)  You don't need the "global" temporar.  Just pass aScope to WrapObject.
4)  s/OBJECT_TO_JSVAL(flat)/JS::ObjectValue(*obj).
5)  If !NS_IsMainThread, you need to actually throw an exception on aCx, not just return
    false, no?

> Name this ThreadSafeGetSafeJSContext.

We have ThreadsafeGetCurrentJSContext (note lowercase "safe").  We should pick one spelling of "safe" and stick with it...

> bool isMainThread = NS_IsMainThread();

Store it in a bool member, so you don't have to NS_IsMainThread a second time in the dtor?
"ThreadSafeGetSafeJSContext" is a misnomer - there's no such thing as the SafeJSContext OMT. The caller should demultiplex the cases, or we should have a helper method with a more specific name like "GetContextForCallbacks" or something.
Created attachment 801034 [details] [diff] [review]
Get WebIDL callbacks working on Workers.

Updated per bz's comments and renamed ThreadSafeGetSafeJSContext() to GetDefaultJSContextForThread() per IRC discussion with bholley.
Attachment #801034 - Flags: review?(khuey)
Attachment #799881 - Attachment is obsolete: true
Attachment #799881 - Flags: review?(khuey)
Comment on attachment 801034 [details] [diff] [review]
Get WebIDL callbacks working on Workers.

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

\o/

::: content/base/src/nsContentUtils.cpp
@@ +1763,5 @@
>  namespace mozilla {
>  namespace dom {
>  namespace workers {
>  extern bool IsCurrentThreadRunningChromeWorker();
> +extern JSContext * GetCurrentThreadJSContext();

nit: JSContext*

@@ +5198,5 @@
>  /* static */
> +JSContext *
> +nsContentUtils::GetDefaultJSContextForThread()
> +{
> +  if (NS_IsMainThread()) {

if (MOZ_LIKELY(NS_IsMainThread()))

::: dom/bindings/BindingUtils.cpp
@@ +635,5 @@
>                                           bool aAllowNativeWrapper)
>  {
>    nsresult rv;
> +  // First, see if this object supports the wrapper cache.
> +  // Note: If |cache->IsDOMBinding()| is true, then it means that the object

This comment is true, but vacuous, since cache->IsDOMBinding() is the only case we care about here.  I'd just write that we're inlining the part of XPCConvert::NativeInterfaceToJSObject that we need on all threads.

@@ +643,5 @@
> +  // object will create (and fill the cache) from its WrapObject call.
> +  nsWrapperCache *cache = aHelper.GetWrapperCache();
> +
> +  if (cache) {
> +    if (cache->IsDOMBinding()) {

collapse the two if statements.

@@ +660,5 @@
> +      }
> +    }
> +  }
> +
> +  // On workers, don't attempt actions that require main thread.

This should just be a crash, imo.

::: dom/bindings/CallbackObject.cpp
@@ +98,5 @@
> +
> +    // Make sure our JSContext is pushed on the stack.
> +    mCxPusher.Push(cx);
> +  } else {
> +    cx = nsContentUtils::GetDefaultJSContextForThread();

I think you should just call the worker function directly since you already know you aren't on the main thread.

@@ +99,5 @@
> +    // Make sure our JSContext is pushed on the stack.
> +    mCxPusher.Push(cx);
> +  } else {
> +    cx = nsContentUtils::GetDefaultJSContextForThread();
> +    xpc_UnmarkGrayContext(cx);

xpc_UnmarkGrayContext is unnecessary and gone since bug 910937.

@@ +183,5 @@
>    mCxPusher.Pop();
> +
> +  if (mIsMainThread) {
> +    nsContentUtils::LeaveMicroTask();
> +  }

Please add a comment that is important that this be last (after we leave the compartment and pop the context).

::: dom/bindings/CallbackObject.h
@@ +152,5 @@
>      // we should re-throw them.
>      ErrorResult& mErrorResult;
>      const ExceptionHandling mExceptionHandling;
>      uint32_t mSavedJSContextOptions;
> +    bool mIsMainThread;

super-nit: const bool

::: dom/workers/RuntimeService.cpp
@@ +1127,5 @@
>  {
>    NS_ASSERTION(!NS_IsMainThread(), "Wrong thread!");
>    CycleCollectedJSRuntime* ccrt = nsCycleCollector_currentJSRuntime();
>    if (!ccrt) {
> +    return NULL;

nullptr.
Attachment #801034 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/2eb63267254c
Status: NEW → RESOLVED
Last Resolved: 5 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.