Closed Bug 778420 Opened 13 years ago Closed 12 years ago

Fix tests that fail with new enablePrivilege implementation

Categories

(Testing :: Mochitest, defect)

Product:
Testing
Component:
Mochitest
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(5 files)

Add Services getter to SpecialPowers. v1
1.54 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Patch by Gijs - Fix test_bug372964. v1 r=bholley
1.07 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Patch by wchen - Initial stab at the tests. v2 r=bholley
38.61 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Fix up tests that don't like the new enablePrivilege. v1
37.74 KB, patch
jmaher
: review+
Details | Diff | Splinter Review
Patch by wchen w/ update to test_offline_gzip
38.59 KB, patch
bholley
: review+
Details | Diff | Splinter Review
In bug 757046, we're making enablePrivilege dumber and test-only. There are some tests that need to be fixed, and this bug is about fixing them.
Attachment #646840 - Flags: review?(jmaher)
Comment on attachment 646840 [details] [diff] [review] Add Services getter to SpecialPowers. v1 Review of attachment 646840 [details] [diff] [review]: ----------------------------------------------------------------- This looks great.
Attachment #646840 - Flags: review?(jmaher) → review+
Comment on attachment 646843 [details] [diff] [review] Fix up tests that don't like the new enablePrivilege. v1 Review of attachment 646843 [details] [diff] [review]: ----------------------------------------------------------------- Nothing too major is concerning me. I would like to address these comments before landing. ::: content/base/test/test_bug422537.html @@ +36,5 @@ > xhr.open("POST", url, true); > + if (i == isupports_string) > + SpecialPowers.wrap(xhr).send(i); > + else > + xhr.send(i); I don't understand why we do this if/else here. Also why do we need to sp.wrap() for the send, and not the open? ::: content/html/content/test/test_bug143220.html @@ +37,5 @@ > } > > function initControl1() { > + SpecialPowers.wrap($("i1")).value = fullPath; > + is(SpecialPowers.wrap($("i1")).value, fullPath, "Should have set full path 1"); do we need to rewrap this? seems inefficient. @@ +42,5 @@ > } > > function initControl2() { > + SpecialPowers.wrap($("i2")).value = fullPath; > + is(SpecialPowers.wrap($("i2")).value, fullPath, "Should have set full path 2"); do we need to rewrap this? ::: content/media/test/manifest.js @@ +162,3 @@ > const Ci = Components.interfaces; > + const Cc = SpecialPowers.wrap(Components).classes; > + const Cr = SpecialPowers.wrap(Components).results; why do you wrap Cc and Cr, but not Ci? @@ +471,4 @@ > // Ensure that preload preferences are comsistent > + var prefService = SpecialPowers.wrap(Components) > + .classes["@mozilla.org/preferences-service;1"] > + .getService(Components.interfaces.nsIPrefService); you have Cc defined above, you should use that here. @@ +491,1 @@ > branch.setIntPref("preload.default", oldDefault); does this still work without the enablePrivilege? we are using branch (which is from prefservice i.e. specialpowers.wrap), but this is executed inside an event listener. Not sure if we need to redefine branch down here. ::: docshell/test/navigation/NavigationUtils.js @@ +102,4 @@ > var Ci = Components.interfaces; > + var ww = SpecialPowers.wrap(Components) > + .classes["@mozilla.org/embedcomp/window-watcher;1"] > + .getService(Ci.nsIWindowWatcher); no need to wrap Ci? ::: docshell/test/navigation/test_bug386782.html @@ +76,1 @@ > gTest.window.history.back(); do we need enablePrivilege for history.back? ::: docshell/test/test_bug529119-1.html @@ +47,5 @@ > { > case 0: > /* 2. We have succeededfully loaded a page, now go to a faulty URL */ > window.setTimeout(function() { > w.location.href = faultyURL; how can we set w.location.href here, but require a wrap(w) later in the file? ::: docshell/test/test_bug529119-2.html @@ +45,4 @@ > /* 2. We have successfully loaded a page, now go to a faulty URL */ > // XXX The test fails when we change the location synchronously > window.setTimeout(function() { > w.location.href = faultyURL; We are setting w.location.href, but requiring SpecialPowers.wrap() to access w.location.href for a is/isnot check later in the file. ::: dom/tests/mochitest/bugs/test_bug317448.html @@ +29,4 @@ > QueryInterface(Components.interfaces.nsIClassInfo). > getInterfaces(count). > map(function(id) { > + id = SpecialPowers.wrap(id); isn't ID just a string? ::: gfx/tests/mochitest/test_acceleration.html @@ +22,5 @@ > > +var importObj = {}; > +SpecialPowers.wrap(Components).utils.import("resource://gre/modules/Services.jsm", > + importObj); > +var Services = SpecialPowers.wrap(importObj).Services; In your other patch, you have the ability to use SpecialPowers.services ?
Attachment #646843 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #6) > Comment on attachment 646843 [details] [diff] [review] > Fix up tests that don't like the new enablePrivilege. v1 > > Review of attachment 646843 [details] [diff] [review]: > ----------------------------------------------------------------- > > Nothing too major is concerning me. I would like to address these comments > before landing. > > ::: content/base/test/test_bug422537.html > @@ +36,5 @@ > > xhr.open("POST", url, true); > > + if (i == isupports_string) > > + SpecialPowers.wrap(xhr).send(i); > > + else > > + xhr.send(i); > > I don't understand why we do this if/else here. the isupports_string codepath requires chrome privileges, but the other tests require the xhr object to inherit the principal of the page. This was the only way this test would pass. > Also why do we need to > sp.wrap() for the send, and not the open? Because we don't fail a security check when doing open(). > do we need to rewrap this? seems inefficient. > > do we need to rewrap this? The overhead or wrapping in a mochitest is inconsequential. I was aiming to keep the difference in behavior as small, readable, and understandable as possible. ;-) > @@ +162,3 @@ > > const Ci = Components.interfaces; > > + const Cc = SpecialPowers.wrap(Components).classes; > > + const Cr = SpecialPowers.wrap(Components).results; > > why do you wrap Cc and Cr, but not Ci? Because Ci is already exposed to unprivileged content. > > @@ +471,4 @@ > > // Ensure that preload preferences are comsistent > > + var prefService = SpecialPowers.wrap(Components) > > + .classes["@mozilla.org/preferences-service;1"] > > + .getService(Components.interfaces.nsIPrefService); > > you have Cc defined above, you should use that here. ok. > > @@ +491,1 @@ > > branch.setIntPref("preload.default", oldDefault); > > does this still work without the enablePrivilege? we are using branch > (which is from prefservice i.e. specialpowers.wrap), but this is executed > inside an event listener. Not sure if we need to redefine branch down here. eventListeners make a difference for enablePrivilege (whose effect is scoped to the duration of the function in which it's called), but make no difference for SpecialPowers.wrap (whose effect depends on it being a separate proxy object). > > ::: docshell/test/navigation/NavigationUtils.js > no need to wrap Ci? nope. :-) > > ::: docshell/test/navigation/test_bug386782.html > @@ +76,1 @@ > > gTest.window.history.back(); > > do we need enablePrivilege for history.back? Nope. It's cross-origin accessible, at least in Gecko. See js/xpconnect/wrappers/AccessCheck.cpp for our implementation of the same-origin policy (I can't seem to find the same-origin policy enumerated anywhere online). > ::: docshell/test/test_bug529119-1.html > how can we set w.location.href here, but require a wrap(w) later in the > file? > > ::: docshell/test/test_bug529119-2.html > @@ +45,4 @@ > > /* 2. We have successfully loaded a page, now go to a faulty URL */ > > // XXX The test fails when we change the location synchronously > > window.setTimeout(function() { > > w.location.href = faultyURL; > > We are setting w.location.href, but requiring SpecialPowers.wrap() to access > w.location.href for a is/isnot check later in the file. location.href is actually cross-origin accessible for writes only (the idea being that cross-origin code ought to be able to navigate a page, but not read its active URL, since anchors and such can leak information). So we need it for reads, but not for writes. > > ::: dom/tests/mochitest/bugs/test_bug317448.html > @@ +29,4 @@ > > QueryInterface(Components.interfaces.nsIClassInfo). > > getInterfaces(count). > > map(function(id) { > > + id = SpecialPowers.wrap(id); > > isn't ID just a string? It's actual an nsiid. And there's some funkiness where id gets implicitly converted with [[DefaultValue]] when passed to the indexed-getter. So we actually need to wrap it. :-( > > ::: gfx/tests/mochitest/test_acceleration.html > @@ +22,5 @@ > > > > +var importObj = {}; > > +SpecialPowers.wrap(Components).utils.import("resource://gre/modules/Services.jsm", > > + importObj); > > +var Services = SpecialPowers.wrap(importObj).Services; > > In your other patch, you have the ability to use SpecialPowers.services ? I fixed this test before adding SpecialPowers.Services. I'll fix this now.
(In reply to Bobby Holley (:bholley) from comment #7) > > you have Cc defined above, you should use that here. > > ok. Er, nevermind. It's defined in function scope, so it's not accessible here.
I updated wchen's patch with a small fix in offline_gzip (first hunk, there were some more instances of Components.classes that the patch missed). Pushed to try with all the other stuff: https://tbpl.mozilla.org/?tree=Try&rev=ee3acd9990bc
Attachment #649133 - Flags: review?(bobbyholley+bmo)
Attachment #649133 - Flags: review?(bobbyholley+bmo) → review+
Push backed out for failures in test_bug435425.html: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4a63a0b90c2 https://hg.mozilla.org/integration/mozilla-inbound/rev/cf99db30a20c Seeing as this has bounced twice, please can there be a full green try run before this lands again (with URL pasted in-bug).
(In reply to Ed Morley [:edmorley] from comment #13) > Push backed out for failures in test_bug435425.html: > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4a63a0b90c2 > > https://hg.mozilla.org/integration/mozilla-inbound/rev/cf99db30a20c > > Seeing as this has bounced twice, please can there be a full green try run > before this lands again (with URL pasted in-bug). This is confusing, because there *was* a try run which didn't hit this orange (comment #11). :-\ Anyway, I'll try to look into what went wrong here later today.
(In reply to Gijs Kruitbosch from comment #14) > (In reply to Ed Morley [:edmorley] from comment #13) > > Push backed out for failures in test_bug435425.html: > > https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=b4a63a0b90c2 > > > > https://hg.mozilla.org/integration/mozilla-inbound/rev/cf99db30a20c > > > > Seeing as this has bounced twice, please can there be a full green try run > > before this lands again (with URL pasted in-bug). > > This is confusing, because there *was* a try run which didn't hit this > orange (comment #11). :-\ > > Anyway, I'll try to look into what went wrong here later today. Erm, looks like the try patch didn't change this test, and what I pushed did. I pushed both patches on bug 726053, and it seems that's not what Bobby did last week. Sorry!
Yeah, the other patch from bug 726053 is bitrotted and will turn the tree orange. We don't need it and might not ever need it. I'll obsolete it now. Gijs, are you easily able to repush what was green on try?
(In reply to Bobby Holley (:bholley) (on vacation until aug 9) from comment #16) > Yeah, the other patch from bug 726053 is bitrotted and will turn the tree > orange. We don't need it and might not ever need it. I'll obsolete it now. > > Gijs, are you easily able to repush what was green on try? Well, at the time I simply used Ed's revert changeset, and for proper credit pushing that didn't seem like a good idea, so it took a little bit. Anyway, I've re-pushed after verifying that what I'm pushing this time around is identical to what I pushed to try at the time (commit everything, up -r-6, branch, patch w/ the try changeset, hg diff -rtip, came up empty) https://hg.mozilla.org/integration/mozilla-inbound/rev/7382e74530ec https://hg.mozilla.org/integration/mozilla-inbound/rev/2d2fc75725d8 https://hg.mozilla.org/integration/mozilla-inbound/rev/9772a4983ec9 https://hg.mozilla.org/integration/mozilla-inbound/rev/fb0d1d0a6837 (and https://hg.mozilla.org/integration/mozilla-inbound/rev/f700556031c7 from bug 726053) As said, this now matches the earlier try run: https://tbpl.mozilla.org/?tree=Try&rev=ee3acd9990bc
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: