Closed
Bug 1038432
Opened 10 years ago
Closed 6 years ago
TEST-UNEXPECTED-FAIL | addon-sdk/tests/test-content-script.test Shared To String Proxies | document.location.toString can be modified - true == false
Categories
(Add-on SDK Graveyard :: General, defect, P2)
Tracking
(Not tracked)
RESOLVED
INCOMPLETE
People
(Reporter: mossop, Assigned: gkrizsanits)
References
Details
Something in Firefox has changed that is causing this test to fail. Unfortunately our tests haven't been running properly in Firefox (bug 1037070) for a little while so the failure didn't show up there. Based on the time that this failure showed up in our tbpl I believe that it was caused by this merge: https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=e1a037c085d1 The test attempts to set a property on document.location.toString from a content script which used to work but fails now. Sounds wrapper related to me. My best guess is that bug 1034262 caused this, does that sound likely? The code that that was originally testing doesn't even exist anymore and I'm not sure if we care about being able to set properties on things like that from content script so this might be a case of just deleting the test and moving along, but it deserves some investigation to understand what has changed and why.
Comment 1•10 years ago
|
||
Commit pushed to master at https://github.com/mozilla/addon-sdk https://github.com/mozilla/addon-sdk/commit/5782d5c1535b6faff5e720587d45f8c5ea74c026 Disabling a failing test to get the tree green again. See bug 1038432 for details.
Reporter | ||
Comment 2•10 years ago
|
||
I've disabled the failing test for now so we can go ahead and get tests on Firefox's tbpl fixed
Comment 3•10 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #0) > Something in Firefox has changed that is causing this test to fail. > Unfortunately our tests haven't been running properly in Firefox (bug > 1037070) for a little while so the failure didn't show up there. > > Based on the time that this failure showed up in our tbpl I believe that it > was caused by this merge: > https://hg.mozilla.org/integration/fx-team/pushloghtml?changeset=e1a037c085d1 > > The test attempts to set a property on document.location.toString from a > content script which used to work but fails now. Sounds wrapper related to > me. > > My best guess is that bug 1034262 caused this, does that sound likely? Probably. The intention was that this wouldn't change any behavior, because we'd either be using nsExpandedPrincipal, or have requiresAddonGlobal(). In the first case, that patch has no effect. In the second case, I modified sandbox.js so that we'd pass wantXrays=false for that, also making the patch have no effect. But there may have been a case that we missed, and this failure certainly looks like a case of getting Xrays where we didn't before. I don't know what the test is really testing though. I'll let gabor weigh in on that.
Assignee | ||
Comment 4•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #3) > I don't know what the test is really testing though. I'll let gabor weigh in > on that. As far as I can tell it's dealing with data URIs, which reminds me a case we might missed, when the window is system. So if you attach a content script to an iframe (with a dataURI) of a system window, we get the system vs system case, we want to waive xrays in that case as well I think. This setup is mainly used for testing imo, so I'm not too worried about it in terms of add-on breakage, but should be fixed I guess.
Comment 5•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #4) > As far as I can tell it's dealing with data URIs, which reminds me a case we > might missed, when the window is system. So if you attach a content script > to an iframe (with a dataURI) of a system window, we get the system vs > system case, we want to waive xrays in that case as well I think. This setup > is mainly used for testing imo, so I'm not too worried about it in terms of > add-on breakage, but should be fixed I guess. If both globals have System Principal, the wantXrays flag should be totally ignored. Are you saying that there's a case where that doesn't happen? If so, can someone attach a reduced testcase?
Comment 6•10 years ago
|
||
(ideally, an xpcshell-test that doesn't depend on all the JP stuff)
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Bobby Holley (:bholley) from comment #5) > If both globals have System Principal, the wantXrays flag should be totally > ignored. I was not aware of that. It was an educated guess, but it seems like I will have to debug it then... it's hard to follow what this test file is doing while keeping my sanity to be honest.
Comment 8•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #7) > (In reply to Bobby Holley (:bholley) from comment #5) > > If both globals have System Principal, the wantXrays flag should be totally > > ignored. > > I was not aware of that. It was an educated guess, but it seems like I will > have to debug it then... it's hard to follow what this test file is doing > while keeping my sanity to be honest. Specifically, see http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#891
Reporter | ||
Comment 9•10 years ago
|
||
Gabor, can you figure out if this is something we need to care about?
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 10•10 years ago
|
||
Yes, but probably only next Monday...
Assignee: nobody → gkrizsanits
Flags: needinfo?(gkrizsanits)
Assignee | ||
Comment 11•10 years ago
|
||
I'm getting a bit confused. In scratch-pad if you do (in browser context, so it's system vs content) content.document.location.toString.blah = 42 that used to work now it is a silent failure. What's the wrapper we use in this case? I thought at first that we might use an OpaqueXrayTrait but isXrayWrapper returns null and it cannot be waived either. Bobby, do you know what do we use here, and why is this not working? I get that we don't want default transparent behavior for JS objects (or function in this case) when system inspecting content, and that document.location is special, but it's weird that there is no option for waive. And this is a document.location thing, with content.document.toString the problem does not exist.
Flags: needinfo?(bobbyholley)
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Dave Townsend [:mossop] from comment #9) > Gabor, can you figure out if this is something we need to care about? From what I've seen so far, it's very unlikely that we should worry about any add-on breakage. It's something else than what we thought the problem is.
Comment 13•10 years ago
|
||
(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #11) > I'm getting a bit confused. In scratch-pad if you do (in browser context, so > it's system vs content) content.document.location.toString.blah = 42 that > used to work now it is a silent failure. What's the wrapper we use in this > case? I thought at first that we might use an OpaqueXrayTrait but > isXrayWrapper returns null and it cannot be waived either. Bobby, do you > know what do we use here, and why is this not working? Hm yes, this appears to be a regression for Location WebIDL, probably related to the Unforgeable stuff. The issue is that the Xrayed functions aren't getting cached on the holder (presumably because they're |own|), so we have: content.document.location.toString != content.document.location.toString Can you file a bug about this and needinfo me?
Flags: needinfo?(bobbyholley)
Reporter | ||
Updated•10 years ago
|
Priority: -- → P2
Comment 14•6 years ago
|
||
Add-on SDK is no longer supported so resolving bugs as INCOMPLETE
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → INCOMPLETE
You need to log in
before you can comment on or make changes to this bug.
Description
•