Closed
Bug 853571
Opened 11 years ago
Closed 11 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)
Add-on SDK Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mossop, Assigned: ochameau)
References
Details
(Keywords: intermittent-failure)
Attachments
(3 files)
2.81 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
3.71 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
2.83 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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?
Comment 1•11 years ago
|
||
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.
Comment 2•11 years ago
|
||
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.
Comment 3•11 years ago
|
||
Alright, tomorrow I'll fly back home, after that I'll cook-up some simpler example.
Updated•11 years ago
|
Keywords: intermittent-failure
Assignee | ||
Comment 4•11 years ago
|
||
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.
Assignee | ||
Comment 5•11 years ago
|
||
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);
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 10•11 years ago
|
||
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.
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 13•11 years ago
|
||
Attachment #728449 -
Flags: review?(bzbarsky)
Comment 14•11 years ago
|
||
Attachment #728450 -
Flags: review?(bzbarsky)
Comment 15•11 years ago
|
||
Attachment #728451 -
Flags: review?(bzbarsky)
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment hidden (Legacy TBPL/Treeherder Robot) |
Comment 18•11 years ago
|
||
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 19•11 years ago
|
||
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 20•11 years ago
|
||
Comment on attachment 728451 [details] [diff] [review] Part 3 - Tests. v1 r=me
Attachment #728451 -
Flags: review?(bzbarsky) → review+
Comment 21•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=bd21daa2bb11
Comment 22•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ed10627bc4ab
Comment 23•11 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e218608de7de remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/42e972cb2941 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/652ffc169141
Comment 24•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e218608de7de https://hg.mozilla.org/mozilla-central/rev/42e972cb2941 https://hg.mozilla.org/mozilla-central/rev/652ffc169141
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•