1291850 - Inline caches don't guard against JSObject::swap on an object in the middle of a prototype chain

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Naomi Smith [:spinda] (non-bmo mail: [e-mail])]

Attachment: test_icswapproto.html

This comment https://dxr.mozilla.org/mozilla-central/source/js/src/jit/IonCaches.cpp#412-417 is incorrect - |GeneratePrototypeGuards| doesn't actually do anything to help protect against |JSObject::swap| when generating a cache involving an object within a prototype chain.

Consider the following prototype chain:



         +---------------------------+
Object   |hasOwnProperty: [function] |
         +-------------^-------------+
                       |
                       |
                       +

                     (...)

         +---------------------------+
         |                           |   [HTMLHeadingElementPrototype]
         +-------------^-------------+
                       |
                       |
         +-------------+-------------+
    x1   |                           |   [HTMLHeadingElement]
         +-------------^-------------+
                       |
                       |
         +-------------+-------------+
    x2   |                           |   [Object.create(x1)]
         +---------------------------+



|x1| is an |h1| element in the current document and |x2| is an object created with |Object.create(x2)|. If an inline cache is generated to read |x2.hasOwnProperty|, it will emit a shape guard on |x2|, a shape guard on |Object|, and a read.

If a script in an |iframe| within the document then calls |document.adoptNode| on |x1|, |x1| in the original compartment is spliced by |JSObject::swap| with a cross-compartment proxy pointing to |x1| in the |iframe|'s compartment. Then say |iframe|'s script sets |hasOwnProperty| on the swapped-in element to |"blue"|. This shadows |Object.hasOwnProperty|, causing a reshape of |Object| *within the |iframe|'s compartment*, intended to invalidate inline caches reading from it.

We now have:



+--[ iframe's compartment ]------------------------------------------------+
|                                                                          |
|                                                                          |
|           +---------------------------+                                  |
|  Object   |hasOwnProperty: [function] |                                  |
|           +-------------^-------------+                                  |
|                         |                                                |
|                         |                                                |
|                         +                                                |
|                                                                          |
|                       (...)                                              |
|                                                                          |
|           +---------------------------+                                  |
|           |                           |   [HTMLHeadingElementPrototype]  |
|           +-------------^-------------+                                  |
|                         |                                                |
|                         |                                                |
|           +-------------+-------------+                                  |
|           |hasOwnProperty: "blue"     |   [HTMLHeadingElement]           |
|           +-------------X-------------+                                  |
|                         X                                                |
|                         X                                                |
+-------------------------X------------------------------------------------+
                          X
                          XXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
+--[ parent's compartment ]----------------------------X-------------------+
|                                                      X                   |
|                                                      X                   |
|           +---------------------------+              X                   |
|      x1   |                           |   [Cross-compartment proxy]      |
|           +-------------^-------------+                                  |
|                         |                                                |
|                         |                                                |
|           +-------------+-------------+                                  |
|      x2   |                           |   [Object.create(x1)]            |
|           +---------------------------+                                  |
|                                                                          |
|                                                                          |
+--------------------------------------------------------------------------+



A subsequent read of |x2.hasOwnProperty| from the parent's compartment should return |"blue"|, from |x1.hasOwnProperty|. However, |Object| within this compartment has *not* been reshaped, so the previously-generated inline cache will check the shape of |x2| (match), check the shape of |Object| (match), and then read |Object.hasOwnProperty|. The code in |GeneratePrototypeGuards| that purports to deal with invalidating the cache in the case of |JSObject::swap| actually has nothing to do with |JSObject::swap|.

The attached mochitest demonstrates the above scenario.

I propose that |JSObject::swap| behave similarly to |SetPropertyByDefining|, which invokes |PurgeScopeChain| to reshape the objects necessary for cache invalidations.

Comment 1

[Naomi Smith [:spinda] (non-bmo mail: [e-mail])]

Attachment: icswapproto-helper.html

Comment 2

[Matthew Gaudet (he/him) [:mgaudet]]

Just reconfirmed this is still an issue.

FAIL Cache stub correctly handles JSObject::swap in the middle of a proto chain - got function hasOwnProperty() {
    [native code]
}, expected "blue"
    SimpleTest.is@SimpleTest/SimpleTest.js:312:5
    @js/xpconnect/tests/mochitest/test_icswapproto.html:38:7
    rval@SimpleTest/SimpleTest.js:146:17
    EventHandlerNonNull*this.addLoadEvent@SimpleTest/SimpleTest.js:171:13
    @SimpleTest/SimpleTest.js:1444:5

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Clearing inactive mentor