789494 - Remove enablePrivilege from mochitests that misbehave with bug 789224

Status: RESOLVED FIXED

(Testing :: General, defect)

Description

[Bobby Holley (:bholley)]

In bug 789224, I'm consolidating the 'are we privileged?' security check code into nsContentUtils::IsCallerChrome, which checks for system principal _and_ UniversalXPConnect. This causes a number of mochitests that use enablePrivilege to break, because they succeed in being chrome where they didn't previously expect to.

Thankfully, enablePrivilege now exists solely for the purposes of making our legacy tests go green, so updating those tests is a perfectly acceptable approach.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Rip enablePrivilege out of misbehaving mochitests. v1

Attaching a patch. Flagging Andrew for review since he's now got some experience with this stuff. :-)

Comment 2

[Bobby Holley (:bholley)]

Pushed this to try along with the patches in bug 788914.

https://tbpl.mozilla.org/?tree=Try&rev=e418fdd6aecb

Comment 3

[Andrew McCreight [:mccr8]]

Comment on attachment 659279 [details] [diff] [review]
Rip enablePrivilege out of misbehaving mochitests. v1

Review of attachment 659279 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/events/test/test_bug448602.html
@@ +105,5 @@
>    // Event target chain tests
>    var l2 = document.getElementById("testlevel2");
>    var l3 = document.getElementById("testlevel3");
>    var textnode = l3.firstChild;
> +  var chain = SpecialPowers.unwrap(els.getEventTargetChainFor(textnode, {}));

Why do you need the unwrap here?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #3)
> Comment on attachment 659279 [details] [diff] [review]
> Rip enablePrivilege out of misbehaving mochitests. v1
> 
> Review of attachment 659279 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/events/test/test_bug448602.html
> @@ +105,5 @@
> >    // Event target chain tests
> >    var l2 = document.getElementById("testlevel2");
> >    var l3 = document.getElementById("testlevel3");
> >    var textnode = l3.firstChild;
> > +  var chain = SpecialPowers.unwrap(els.getEventTargetChainFor(textnode, {}));
> 
> Why do you need the unwrap here?

Because the subsequent code does a lot of identity comparisons with chain[i], which end up being SpecialPowers wrappers without the unwrap (meaning that they fail the identity checks).

Thanks for the quick review, Andrew!

Comment 5

[Bobby Holley (:bholley)]

Looks green. Pushed to m-i:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b6bc9c9269

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Sorry, this had to be backed out due to assertion failures from bug 788914.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fc73c2c680d1

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e8b6bc9c9269

Comment 8

[Ryan VanderMeulen [:RyanVM]]

I inadvertently merged this over to m-c. And then backed it back out.
https://hg.mozilla.org/mozilla-central/rev/591eb47f6bd3

Comment 9

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9a39d8cb59d

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b9a39d8cb59d