996208 - Methods !== themselves in content scripts

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Jordan Santell [:jsantell] [@jsantell]]

From a bug found in bug 990762, when removing our version of the global postMessage, there was a separate test testing:

`postMessage === postMessage`

which failed after changing this. Matteo investigated further confirming objects and constructors are fine (window.Function === window.Function) but methods fail (window.alert !== window.alert)

Comment 1

[Jordan Santell [:jsantell] [@jsantell]]

Pinging you Gabor on this!

Comment 2

[Matteo Ferretti [:zer0] [:matteo]]

More weird (from content script):

    window.postMessage.foo = {bar: 1};
    
    console.log(window.postMessage.foo.bar); // 1
    console.log(window.postMessage.foo === window.postMessage.foo) // true
    console.log(window.postMessage === window.postMessage) // false

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

Copied from IRC:

> <Mossop> ZER0: window.postMessage is probably a getter returning a wrapped function or something that would differ everytime. I bet |let foo = postMessage; foo === foo| works fine

This seems the only logical explanation, I thought I covered that with the code in comment 2, but I didn't consider that getting and setting properties on a wrapper can do it to the underlying object.

Said that, it feels wrong to be wrapping the window's methods every time.

Comment 4

[Gabor Krizsanits (INACTIVE)]

This is another side effect of our window getter hack. But this seems like a platform bug non the less, since the same bug could be produced by instead of window.postMessage calling [this.]postMessage. The sandbox proxy creates a new proxy for each property get operation it seems: http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#817 to fix up the |this| pointer in these calls. But that breaks the identity operator. Bobby, I don't see any better solution here than some kind of caching... probably by defining property on the sandbox after the first resolve of such callable props. What do you think?

Comment 5

[Bobby Holley (:bholley)]

Yeah, if we want to fix this, we need some sort of caching. I think defining it on the global is likely not the right approach though. We need to avoid letting the cache grow stale, especially in the non-Xray case.

I think the solution is to have a Holder object that hangs off the sandboxProxyHandler, and maps from propertyNames => remappedCallable. Then, when we do a lookup, we check the holder, and see if the underlying callable matches the result we'd pull off of the target object. If so, we return the cached version.

This is kind of a pain to do, so we should only do it if people are actually complaining about it.

Comment 6

[Gabor Krizsanits (INACTIVE)]

I'm not too wild about implementing a complex machinery there if it's not needed either. The whole sandbox proxy handler magic is kind of a PIA already, so I would love to avoid adding more complexity to it for very little benefit. Jordan, how important is to fix this? Can we just won't fix it until someone starts screaming about it for some important reason?

Comment 7

[Jordan Santell [:jsantell] [@jsantell]]

Not critical, just may cause some confusion for addon developers who are debugging.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Alright, let's leave it open for now.

Comment 9

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562