Closed Bug 853571 Opened 7 years ago Closed 7 years ago

TEST-UNEXPECTED-FAIL | tests/test-content-proxy.test Object Listener 2 | event.source is the wrapped window - true == false

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mossop, Assigned: ochameau)

References

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

TEST-UNEXPECTED-FAIL | tests/test-content-proxy.test Object Listener 2 | event.source is the wrapped window - true == false
TEST-UNEXPECTED-FAIL | tests/test-content-proxy.test postMessage | event.source is the top window

Alex, Bobby has broken our content-proxy tests with something in here https://tbpl.mozilla.org/?tree=Mozilla-Inbound&showall=1&rev=b0a7e6aee6d7

Can you figure out what is going on. Do we still need the content-proxy tests?
Blocks: 629263
Just for the fun of it I tried to figure out what might happen here... The second test is very interesting in itself. https://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/test/test-content-proxy.js#201

The bug testing the following if I get it right, please Alex do confirm it or correct me if I'm wrong. There is a content window with an iframe. There is a sandbox with the window as its proto and with wantXray on (the worker). We post a message from the sandbox to the iframe like this: 
document.getElementById("iframe").contentWindow.postMessage(json, "*");
(again, everything is an xray here)
and then we listen to this message from the iframe, and in that listener function we check if event.source is the top level window (it should be since the scope we send the message from is the sandbox with the window as its proto)

There is one more trick that should not matter, but I mention it anyway, that we send the even from a 'new function(){...}'.

So first of all I have no idea about the binding of postMessage but I assume it is a special case. Then there is this CallerInnerWindow that the whole .source attribute is computed from... this function gives me stress, I really don't like these kind of context stack magic :( Anyway, Bobby, if I recall correctly you did some changes around the sandbox prototype |this| calculation could that be related? Or the cleanup around the context stack? 

I am not 100% sure that I deciphered this test correctly, but very likely.
Yeah, looks like PostMessage does all kind of crazy stuff. If you can post a reduced testcase with less proxy magic, I'm happy to take a look.
Alright, tomorrow I'll fly back home, after that I'll cook-up some simpler example.
The name of this test is misleading as it just test content script features.
But I named it like this as it originaly verified that we don't regress all proxies workaround.

So there is no proxy magic, but I can easily agree that our content-script/worker stuff isn't much more easy to strip down.
The "test postMessage" failure is about event.source being null.
Before your patch, we were used to get the content script related window.

Here is a minimal scratchpad testcase (to execute with a http: webpage opened in current tab):
  let Cu = Components.utils;

  content.addEventListener("message", function (event) {
    // Here `event.source` used to be `content`,
    // Now, it is null
    Cu.reportError(event.source);
  });

  let s = Cu.Sandbox(content, {sandboxPrototype:content});
  s.iframe = iframe;
  Cu.evalInSandbox("window.postMessage('foo', '*');", s);
So, the basic issue here is that the content script is expecting to be able to impersonate its sandboxPrototype for the purposes of postMessage event.source. This is kind of sketchy, but I see the use case. FWIW, this only worked before because of a quirk in the GWNOJO call in GetCallerWindow whereby we'd end up climbing the prototype chain of the Sandbox, unwrapping, and finding the native window. Thank heavens we're not doing that anymore.

Regardless, bz and I think it's probably worth hacking in support for this to make content scripts work better. I'll write a patch.
Attachment #728451 - Flags: review?(bzbarsky)
Comment on attachment 728449 [details] [diff] [review]
Part 1 - Add some belt-and-suspenders checks for sketchy scripted caller detection. v1

r=me
Attachment #728449 - Flags: review?(bzbarsky) → review+
Comment on attachment 728450 [details] [diff] [review]
Part 2 - Do special detection for sandboxPrototype to make sure the source gets set up right in postMessage. v1

Ick, but yeah.
Attachment #728450 - Flags: review?(bzbarsky) → review+
Comment on attachment 728451 [details] [diff] [review]
Part 3 - Tests. v1

r=me
Attachment #728451 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.