Closed Bug 1316616 Opened 3 years ago Closed 3 years ago

Decide on an API for whether caller is chrome

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

See discussion in bug 1316480 comment 2 and following.  What API would spidermonkey be ok with here?
OK, talked to some SpiderMonkey folks (waldo, shu).  They're not super-happy about adding stuff in SpiderMonkey, so I guess we get to do this ourselves.  Given that, I'm not sure anything is worth doing here.

I mean, we could do something like this:

1)  Extend our existing context private with a tristate as suggested in bug 1316480 comment 4.
2)  Have a function that gets the context private, checks the tristate,
    if it's "check principal" checks the compartment principal.

Or we could basically keep our existing logic:

1)  Check NS_IsMainThread().
2)  If true, check compartment principal.
3)  If false, call IsCurrentThreadRunningChromeWorker() or equivalent.

and just wrap it up in a function that takes JSContext*.

I guess the benefit of doing the former would be slightly fewer includes (e.g. no worker-specific includes needed, but needs BindingUtils.h) and a bit less work in the worker case...  Either way we end up with at least one out of line function call.
Component: JavaScript Engine → DOM
Are any of the webidl methods with this annotation hot enough to worry about the impact of the TLS lookup? I'm tempted to just leave as-is unless it's a known bottleneck.
So far, no.  Some are hot (e.g. HTMLInputElement value setter, dispatchEvent) but also do so much work a TLS lookup is noise.  Once we add this thing we'll use it in some *IsEnabled checks in Gecko too.

Maybe I should go ahead and stop worrying about the performance and just focus on making the include situation least sucky.  In practice that means an nsContentUtils method or something along those lines.  Maybe just:

  bool
  nsContentUtils::IsCallerSystem(JSContext* aCx)
  {
    return nsJSPrincipals::get(JS_GetCompartmentPrincipals(JS_GetContextCompartment(aCx))) == sSystemPrincipal;
  }

  bool
  nsContentUtils::ThreadsafeIsCallerSystem(JSContext* aCx)
  {
    if (NS_IsMainThread()) {
      return IsCallerChrome(aCx);
    }

    return workers::IsCurrentThreadRunningChromeWorker();
  }

and maybe add nsContentUtils::IsObjectSystem.  Or have the getters return a CallerType instead of bool, I guess.
Priority: -- → P3
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8811833 - Flags: review?(bobbyholley) → review+
Attachment #8811834 - Flags: review?(bobbyholley) → review+
Though I'm not totally sure why we're not returning a bool here. A predicate function would be easier to use and I'm not sure what the enum buys us.
That's one of the things I wanted you to review, yes.

For binding _callees_ we're passing an enum for the caller type because that avoids the "boolean argument" readability problem.

But for places that just need a predicate, we could easily have a boolean-returning one.  So maybe the nsContentUtils API should return boolean and the binding code in CGCallGenerator should convert that to a CallerType enum value....
Flags: needinfo?(bobbyholley)
And if we do it that way, I'd probably want "IsSystemCaller" and "ThreadsafeIsSystemCaller" as the predicated names....
Comment on attachment 8811832 [details] [diff] [review]
part 1.  Replace some redundant code in GetCurrentThreadWorkerPrivate() with a call to a function that we already have

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

::: dom/workers/RuntimeService.cpp
@@ -1458,5 @@
> -  if (!cxPrivate) {
> -    // This can happen if the nsCycleCollector_shutdown() in ~WorkerJSContext()
> -    // triggers any calls to GetCurrentThreadWorkerPrivate().  At this stage
> -    // CycleCollectedJSContext::Get() will still return a context, but
> -    // the context private has already been cleared.

Can you preserve this comment above your call to GetWorkerPrivateFromContext()?  Maybe changing it to start "Note that we can return nullptr if the nsCycleCollector_shutdown()...".
Attachment #8811832 - Flags: review?(bkelly) → review+
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #9)
> And if we do it that way, I'd probably want "IsSystemCaller" and
> "ThreadsafeIsSystemCaller" as the predicated names....

Yep, those sound good.
Flags: needinfo?(bobbyholley)
> Can you preserve this comment above your call to GetWorkerPrivateFromContext()?

Will do.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e70b0fb180a3
part 1.  Replace some redundant code in GetCurrentThreadWorkerPrivate() with a call to a function that we already have.  r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/d9c4800ffba6
part 2.  Add an nsContentUtils API for getting the CallerType of a JSContext.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/491ce96f6547
part 3.  Use the new CallerType-getting API in a few places to demonstrate how it works.  r=bholley
https://hg.mozilla.org/mozilla-central/rev/e70b0fb180a3
https://hg.mozilla.org/mozilla-central/rev/d9c4800ffba6
https://hg.mozilla.org/mozilla-central/rev/491ce96f6547
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1324035
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.