1038432 - TEST-UNEXPECTED-FAIL | addon-sdk/tests/test-content-script.test Shared To String Proxies | document.location.toString can be modified - true == false

Status: RESOLVED INCOMPLETE

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

Description

[Dave Townsend [:mossop]]

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

[[github robot]]

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.

Comment 2

[Dave Townsend [:mossop]]

I've disabled the failing test for now so we can go ahead and get tests on Firefox's tbpl fixed

Comment 3

[Bobby Holley (:bholley)]

(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.

Comment 4

[Gabor Krizsanits (INACTIVE)]

(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

[Bobby Holley (:bholley)]

(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

[Bobby Holley (:bholley)]

(ideally, an xpcshell-test that doesn't depend on all the JP stuff)

Comment 7

[Gabor Krizsanits (INACTIVE)]

(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

[Bobby Holley (:bholley)]

(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

Comment 9

[Dave Townsend [:mossop]]

Gabor, can you figure out if this is something we need to care about?

Comment 10

[Gabor Krizsanits (INACTIVE)]

Yes, but probably only next Monday...

Comment 11

[Gabor Krizsanits (INACTIVE)]

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.

Comment 12

[Gabor Krizsanits (INACTIVE)]

(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

[Bobby Holley (:bholley)]

(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?

Comment 14

[BMO Automation]

Add-on SDK is no longer supported so resolving bugs as INCOMPLETE