Commit 9e02f38a authored by Emilio Cobos Álvarez's avatar Emilio Cobos Álvarez
Browse files

Bug 1766909 - Wrap ObservableArray proxy on getter, rather than throwing. r=peterv 

This will still prevent them from accessing stuff (.length will be
undefined, etc), but seems better than unexpectedly throwing. This fixes
the issue at hand at least.

With this patch, we reject length accesses here:

  https://searchfox.org/mozilla-central/rev/86c98c486f03b598d0f80356b69163fd400ec8aa/js/xpconnect/wrappers/XrayWrapper.cpp#229-233

Your call on whether this patch is enough as-is, or more work is needed.
Also your call on whether if more work is needed that needs to happen on
this bug or somewhere else.

I'm not sure what we'd need to do to support this more "properly",
presumably we'd need to add special XRay support to
ObservableArrayProxyHandler or so? Pointers (or patches of course ;))
welcome.

Also unsure about the setter situation, I _think_ it's fine not to throw
given the code I read, but please sanity-check.

Differential Revision: https://phabricator.services.mozilla.com/D145045
parent 218b3891

dom/bindings/BindingUtils.cpp

+11 −4
Original line number Diff line number Diff line
@@ -3531,22 +3531,29 @@ bool GetSetlikeBackingObject(JSContext* aCx, JS::Handle<JSObject*> aObj, 
}



static inline JSObject* NewObservableArrayProxyObject(

    JSContext* aCx, const ObservableArrayProxyHandler* aHandler) {

    JSContext* aCx, const ObservableArrayProxyHandler* aHandler, void* aOwner) {

  JS::RootedObject target(aCx, JS::NewArrayObject(aCx, 0));

  if (NS_WARN_IF(!target)) {

    return nullptr;

  }



  JS::RootedValue targetValue(aCx, JS::ObjectValue(*target));

  return js::NewProxyObject(aCx, aHandler, targetValue, nullptr);

  JS::RootedObject proxy(

      aCx, js::NewProxyObject(aCx, aHandler, targetValue, nullptr));

  if (!proxy) {

    return nullptr;

  }

  js::SetProxyReservedSlot(proxy, OBSERVABLE_ARRAY_DOM_INTERFACE_SLOT,

                           JS::PrivateValue(aOwner));

  return proxy;

}



bool GetObservableArrayBackingObject(

    JSContext* aCx, JS::Handle<JSObject*> aObj, size_t aSlotIndex,

    JS::MutableHandle<JSObject*> aBackingObj, bool* aBackingObjCreated,

    const ObservableArrayProxyHandler* aHandler) {

    const ObservableArrayProxyHandler* aHandler, void* aOwner) {

  return GetBackingObject<NewObservableArrayProxyObject>(

      aCx, aObj, aSlotIndex, aBackingObj, aBackingObjCreated, aHandler);

      aCx, aObj, aSlotIndex, aBackingObj, aBackingObjCreated, aHandler, aOwner);

}



bool ForEachHandler(JSContext* aCx, unsigned aArgc, JS::Value* aVp) {

dom/bindings/BindingUtils.h

+1 −1
Original line number Diff line number Diff line
@@ -3127,7 +3127,7 @@ bool GetSetlikeBackingObject(JSContext* aCx, JS::Handle<JSObject*> aObj, 
bool GetObservableArrayBackingObject(

    JSContext* aCx, JS::Handle<JSObject*> aObj, size_t aSlotIndex,

    JS::MutableHandle<JSObject*> aBackingObj, bool* aBackingObjCreated,

    const ObservableArrayProxyHandler* aHandler);

    const ObservableArrayProxyHandler* aHandler, void* aOwner);



// Get the desired prototype object for an object construction from the given

// CallArgs.  The CallArgs must be for a constructor call.  The

dom/bindings/Codegen.py

+5 −8
Original line number Diff line number Diff line
@@ -22076,19 +22076,20 @@ def getObservableArrayBackingObject(descriptor, attr, errorReturn="return false; 
    assert attr.isAttr()

    assert attr.type.isObservableArray()









    # GetObservableArrayBackingObject may return a wrapped object for Xrays, so









    # when we create it we need to unwrap it to store the interface in the









    # reserved slot.









    return fill(









        """









        JS::Rooted<JSObject*> backingObj(cx);









        bool created = false;









        if (!GetObservableArrayBackingObject(cx, obj, ${slot},









                &backingObj, &created, ${namespace}::ObservableArrayProxyHandler::getInstance())) {









                &backingObj, &created, ${namespace}::ObservableArrayProxyHandler::getInstance(),









                self)) {









          $*{errorReturn}









        }









        if (created) {









          PreserveWrapper(self);









          js::SetProxyReservedSlot(backingObj,









                                   OBSERVABLE_ARRAY_DOM_INTERFACE_SLOT,









                                   JS::PrivateValue(self));









        }









        """,









        namespace=toBindingNamespace(MakeNativeName(attr.identifier.name)),








@@ -22105,10 +22106,6 @@ def getObservableArrayGetterBody(descriptor, attr):







    assert attr.type.isObservableArray()









    return fill(









        """









        if (xpc::WrapperFactory::IsXrayWrapper(obj)) {









          JS_ReportErrorASCII(cx, "Accessing from Xray wrapper is not supported.");









          return false;









        }









        $*{getBackingObj}









        MOZ_ASSERT(!JS_IsExceptionPending(cx));









        args.rval().setObject(*backingObj);

























dom/bindings/JSSlots.h



+1
−1






























Original line number
Diff line number
Diff line







@@ -32,7 +32,7 @@







#define DOM_INTERFACE_PROTO_SLOTS_BASE 0



















// The slot index of raw pointer of dom object stored in observable array exotic









// object. We nedd this in order to call the OnSet* and OnDelete* callbacks.









// object. We need this in order to call the OnSet* and OnDelete* callbacks.









#define OBSERVABLE_ARRAY_DOM_INTERFACE_SLOT 0



















// The slot index of backing list stored in observable array exotic object.
































dom/bindings/test/test_observablearray.html



+23
−0






























Original line number
Diff line number
Diff line







@@ -518,6 +518,29 @@ add_task(function testObservableArray_object() {







    isDeeply(values[i], b[i], `check index ${i}`);









  }









});



















add_task(function testObservableArray_xrays() {









  let m = new TestInterfaceObservableArray({









    setObjectCallback(value, index) {









      ok(false, "Shouldn't reach setObjectCallback");









    },









    deleteObjectCallback(value, index) {









      ok(false, "Shouldn't reach deleteObjectCallback");









    },









  });



















  let wrapper = SpecialPowers.wrap(m);









  ok(SpecialPowers.Cu.isXrayWrapper(wrapper), "Should be a wrapper");









  let observableArray = wrapper.observableArrayBoolean;









  ok(!!observableArray, "Should not throw");









  is("length" in observableArray, false, "Should be opaque");



















  try {









    wrapper.observableArrayBoolean = [true, false, false];









    ok(false, "Expected to throw, for now");









  } catch (ex) {}









});



















</script>









</body>









</html>