Kris Maglione [:kmag] Reporter
	Description • 11 years ago

When a PDF is loaded into the current page with PDF.js, Object.getOwnPropertyNames(content) fails with NS_ERROR_FAILURE.

Object.getOwnPropertyNames(content.wrappedJSObject) works, but the following:

    [k for (k in Object.getOwnPropertyNames(content.wrappedJSObject)) if (k in content)]

fails with NS_ERROR_FAILURE for the (k in content) check for the following Worker properties:

    Worker
    WorkerErrorEvent
    WorkerEvent
    WorkerMessageEvent

It's possible that this may fail for other content pages, but I've only seen it with PDF.js so far.

	Kris Maglione [:kmag] Reporter
	Comment 1 • 11 years ago

I'm seeing this in 17, 20.0.1, and recent nightlies.

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 11 years ago

Simple steps to reproduce:

1)  Set devtools.chrome.enabled to true
2)  Load a PDF (make sure pdf.js is on)
3)  Open scratchpad, switch it to browser mode
4)  Evaluate the string "'Worker' in content"

This throws.

Seems like an Xray problem?

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 11 years ago

So we land in mozilla::dom::workers::ResolveWorkerClasses (called from nsWindowSH::NewResolve), coming from an Xray.

This code does:

  JSObject* eventTarget = EventTargetBinding_workers::GetProtoObject(aCx, aObj);

which returns null, so we return false and then nsWindowSH::NewResolve returns NS_ERROR_FAILURE.

And the reason GetProtoObject is returning null is that GetProtoObject starts off with:

145         if (!(js::GetObjectClass(aGlobal)->flags & JSCLASS_DOM_GLOBAL)) {
146           return NULL;
147         }

In this case aGlobal is an xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::XPCWrappedNativeXrayTraits> so of course it's not a JSCLAS_DOM_GLOBAL.

So there are a few issues here:

1)  Worker stuff uses the wrong event target binding, more or less known.
2)  It's not resolving it properly.

	Boris Zbarsky [:bzbarsky]
	Comment 4 • 11 years ago

In particular, compare this code for normal interface objects:

      Maybe<JSAutoCompartment> ac;
      bool defineOnXray = ObjectIsNativeWrapper(cx, obj);
      if (defineOnXray) {
        global = js::CheckedUnwrap(obj, /* stopAtOuter = */ false);

etc.  _Then_ it calls the function that would define the property, using the actual global,  and then does:

        if (defineOnXray) {
          // This really should be handled by the Xray for the window.
          ac.destroy();
          if (!JS_WrapObject(cx, &interfaceObject) ||
              !JS_DefinePropertyById(cx, obj, id,
                                     JS::ObjectValue(*interfaceObject), JS_PropertyStub,
                                     JS_StrictPropertyStub, 0)) {

Presumably the worker bits need to do something similar?

	Bobby Holley (:bholley)
	Comment 5 • 11 years ago

(In reply to Boris Zbarsky (:bz) from comment #4)
> Presumably the worker bits need to do something similar?

Kinda, but that code is totally clunky (I think Andrew has been looking into fixing it). IMO, we don't need any special conditionals or Maybe<>s for the Xray case. We should just need to do:
{
  global = CheckedUnwrap(obj);
  AutoCompartment ac(cx, global);
  interface = createInterfaceObject(cx, ...);
}
JS_WrapObject(cx, interfaceObject);
JS_DefineProperty(cx, obj, interfaceObject, ...)


The code is the way it is because we use the same function to both create the constructor and define it on the global. Really this is wrong per Xray semantics, since resolving it from the Xray side shouldn't cause it to be resolved on the content side. But XPCWN Xrays are kind of limping along anyway, so I'm ok with whatever is the quickest path to victory here.

	Andrew McCreight [:mccr8]
	Comment 6 • 11 years ago

Yeah, I need to write a similar glob of code in two other places (including for WebIDL on navigator), and I was going to rewrite the window case once those were reviewed so that I knew I was on the right track.  Sadly these don't seem to be quite uniform enough to write some kind of helper class to make it easy.  The "create an object" and "define a property" parts aren't uniform.

	Boris Zbarsky [:bzbarsky]
	Comment 7 • 11 years ago

Hmm.  This will be a problem for bug 892687, where that null return becomes a MOZ_CRASH.

Ben, who can look at the worker end of this?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 8 • 11 years ago

This sounds right up khuey's alley!

	Kyle Huey (Exited; not receiving bugmail, old account, do not use) Assignee
	Comment 9 • 11 years ago

Nikhil, would you be interested in working on this?

	Boris Zbarsky [:bzbarsky]
	Comment 10 • 11 years ago

Let's try actually asking nsm.  ;)

	Masatoshi Kimura [:emk]
	Comment 11 • 11 years ago

Is this still a problem even after bug 919885 is fixed?

	Masatoshi Kimura [:emk]
	Comment 12 • 11 years ago

(In reply to Boris Zbarsky [:bz] from comment #2)
> Simple steps to reproduce:
> 
> 1)  Set devtools.chrome.enabled to true
> 2)  Load a PDF (make sure pdf.js is on)
> 3)  Open scratchpad, switch it to browser mode
> 4)  Evaluate the string "'Worker' in content"
> 
> This throws.
> 
> Seems like an Xray problem?

I was unable to reproduce the issue with the latest Nightly anymore.

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 11 years ago

Hmm.  Yeah, we resolve all that stuff over in classinfo now, so things seem ok; the code in question is only reached for non-Window globals, which can't have Xrays.