Closed Bug 901291 Opened 6 years ago Closed 6 years ago

Get WebIDL callbacks working on workers

Categories

(Core :: DOM: Workers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: khuey, Assigned: nsm)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Get callbacks on workers (obsolete) — Splinter Review
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.
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)
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.
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
Closed: 6 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.