Closed
Bug 913706
Opened 8 years ago
Closed 8 years ago
Fix the tests for B2G mochitest that use nsICookiePermission.setAccess
Categories
(Testing :: Mochitest, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla28
People
(Reporter: martijn.martijn, Assigned: martijn.martijn)
References
(Depends on 1 open bug)
Details
Attachments
(1 file, 9 obsolete files)
62.80 KB,
patch
|
Details | Diff | Splinter Review |
There are some tests that use nsICookiePermission.setAccess: dom/tests/mochitest/localstorage/test_cookieBlock.html dom/tests/mochitest/localstorage/frameQuotaSessionOnly.html dom/tests/mochitest/localstorage/test_cookieSession-phase1.html dom/tests/mochitest/localstorage/test_cookieSession-phase2.html dom/tests/mochitest/localstorage/test_localStorageQuotaSessionOnly.html dom/tests/mochitest/localstorage/test_localStorageBaseSessionOnly.html dom/tests/mochitest/localstorage/test_localStorageQuotaSessionOnly2.html dom/tests/mochitest/sessionstorage/test_cookieSession.html dom/tests/mochitest/sessionstorage/test_sessionStorageBaseSessionOnly.html Those tests don't work in B2G mochitest, because this can't be done in the content process. I noticed that internally, this method is setting the cookie permission: http://mxr.mozilla.org/mozilla-central/source/extensions/cookie/nsCookiePermission.cpp#135 So could this code be replaced by using something like SpecialPowers.pushPermissions([{'type': 'cookie', 'allow': true, 'context': document}], startTest); ?
![]() |
||
Comment 1•8 years ago
|
||
Definitely could, just not sure about the *Quota* tests w/o looking at it in more details. Are you willing to do the change and let me review or should I do it?
Assignee | ||
Comment 3•8 years ago
|
||
Ok, I have something that is almost ready, I think. I found 2 problems: - test_cookieSession-phase2.html expects that test_cookieSession-phase1.html is run beforehand. Test files should be written as standalone tests, they shouldn't depend on test files that are run before. Can I rewrite these tests into one test that loads iframes or opens windows or something like that? - test_localStorageQuota.html is giving failures when running as a standalone test, but not when running the whole localstorage/ test directory.
Attachment #801755 -
Flags: feedback?(honzab.moz)
![]() |
||
Comment 4•8 years ago
|
||
(In reply to Martijn Wargers [:mwargers] (QA) from comment #3) > Can I rewrite these > tests into one test that loads iframes or opens windows or something like > that? Feel free :)
![]() |
||
Comment 5•8 years ago
|
||
Comment on attachment 801755 [details] [diff] [review] 913706.diff Review of attachment 801755 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/localstorage/test_cookieSession-phase1.html @@ +18,5 @@ > { > localStorage.setItem("persistent1", "persistent value 1"); > localStorage.setItem("persistent2", "persistent value 2"); > > + //xxx pushPermissions should be used here, but testcookieSession-phase2.html depends on this addPermission IIRC, I've split the test to make sure the localStorage object is threw away between the tests. If you want to put it to a single test, you may need to run each now separate part in its own iframe. On the other hand, the code has changed (bug 600307) and it may no longer be needed to keep this test split at all. ::: dom/tests/mochitest/localstorage/test_cookieSession-phase2.html @@ -70,4 @@ > } > > SimpleTest.waitForExplicitFinish(); > - Leave this blank line. ::: dom/tests/mochitest/localstorage/test_localStorageCookieSettings.html @@ +11,3 @@ > > // Set cookies behavior to "always reject". > +SpecialPowers.pushPrefEnv({"set": [["network.cookie.cookieBehavior", 2]]}, test1); Can you split this nicely to more then one line please? @@ +21,5 @@ > + is(ex.name, "SecurityError"); > + } > + > + // Set cookies behavior to "ask every time". > + SpecialPowers.pushPrefEnv({"set": [["network.cookie.lifetimePolicy", 1]], "clear": [["network.cookie.cookieBehavior"]]}, test2); And here as well. ::: dom/tests/mochitest/localstorage/test_localStorageQuota.html @@ +6,5 @@ > <script type="text/javascript" src="interOriginTest.js"></script> > <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" /> > > <script type="text/javascript"> > +//Note: this test is currently giving failures when running standalone Do you want to open a bug for it?
Attachment #801755 -
Flags: feedback?(honzab.moz) → feedback+
Assignee | ||
Comment 6•8 years ago
|
||
Ok, I changed it to so that each part runs in its own iframe. I addressed your other comments. I also removed the test files from the exclude list of b2g.json in this patch. I'll push this patch to tryserver (as soon as tryserver works again) to see if everything passes and if tests are failing on b2g.json, I'll readd them to the exclude list. (In reply to Honza Bambas (:mayhemer) from comment #5) > > +//Note: this test is currently giving failures when running standalone > > Do you want to open a bug for it? Ok, I filed bug 914865 for it (also mentioned the bug number in the test file.
Attachment #801755 -
Attachment is obsolete: true
Attachment #802646 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 7•8 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=062aca05a6b1
Assignee | ||
Updated•8 years ago
|
Attachment #802646 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 8•8 years ago
|
||
Something went wrong, this patch should be ok now. Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=d5bafdf349bb
Attachment #802646 -
Attachment is obsolete: true
Assignee | ||
Comment 9•8 years ago
|
||
Comment on attachment 802873 [details] [diff] [review] 913706_4.diff Review of attachment 802873 [details] [diff] [review]: ----------------------------------------------------------------- Ok, please ignore the b2g.json changes, I'll remove those in a later patch. The try run gave all kinds of failures and time outs there. I'll look at those later.
Attachment #802873 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 10•8 years ago
|
||
Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=a7d5ffcf8356 Now I got these tests working: dom/tests/mochitest/localstorage/test_cookieBlock.html dom/tests/mochitest/localstorage/test_localStorageBaseSessionOnly.html dom/tests/mochitest/localstorage/test_localStorageReplace.html dom/tests/mochitest/localstorage/test_localStorageEnablePref.html dom/tests/mochitest/sessionstorage/test_sessionStorageBaseSessionOnly.html dom/tests/mochitest/localstorage/test_localStorageCookieSettings.html These still suffer from "no storage chrome event received": dom/tests/mochitest/localstorage/test_localStorageBase.html dom/tests/mochitest/sessionstorage/test_sessionStorageBase.html These now suffer from bug 907770: dom/tests/mochitest/localstorage/test_localStorageQuotaSessionOnly.html dom/tests/mochitest/localstorage/test_localStorageQuotaSessionOnly2.html dom/tests/mochitest/localstorage/test_localStorageQuota.html dom/tests/mochitest/localstorage/test_localStorageOriginsSchemaDiffs.html These test files both have 4 failures on b2g with this patch, for which I don't know, why this happens: dom/tests/mochitest/localstorage/test_cookieSession dom/tests/mochitest/sessionstorage/test_cookieSession.html For some of the cookie permission setting, I had to reload a document to get not worse results (like multiple calling SimpleTest.finish()). I haven't converted all the tests, only where I noticed this issue happening on B2G. It might be better to convert all the test files that way.
Attachment #802873 -
Attachment is obsolete: true
Attachment #802873 -
Flags: review?(honzab.moz)
Attachment #805309 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 11•8 years ago
|
||
I busted tryserver, because I forgot to hg add dom/tests/mochitest/localstorage/test_cookieSession.html Every time I use this patch, I have to do afterwards: hg add dom/tests/mochitest/localstorage/test_cookieSession.html hg rename dom/tests/mochitest/localstorage/test_cookieSession-phase1.html dom/tests/mochitest/localstorage/frameCookieSession-phase1.html hg rename dom/tests/mochitest/localstorage/test_cookieSession-phase2.html dom/tests/mochitest/localstorage/frameCookieSession-phase2.html Because 'patch' doesn't seem to pick those changes up. It's really annoying and it caused me to push the wrong patch. Anyway, new try push with this patch (which is hopefully complete now): https://tbpl.mozilla.org/?tree=Try&rev=0d2009ffbe6d
Attachment #805309 -
Attachment is obsolete: true
Attachment #805309 -
Flags: review?(honzab.moz)
Attachment #805347 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 12•8 years ago
|
||
Tryserver is green.
![]() |
||
Comment 13•8 years ago
|
||
Comment on attachment 805347 [details] [diff] [review] 913706_5.diff Review of attachment 805347 [details] [diff] [review]: ----------------------------------------------------------------- r- to be able to refer the comments. I'd like to check a newer version ones more. In general: more comments in the test, what doesn't fit to be put as comments to the code, add to the comment when attaching the patch. The more explanation I get the quicker and better the review is. ::: dom/tests/mochitest/localstorage/frameCookieSession-phase1.html @@ +25,5 @@ > +function test1() { > + localStorage.setItem("session only", "session value"); > + parent.is(localStorage.getItem("session only"), "session value"); > + parent.is(localStorage.getItem("persistent1"), "persistent value 1"); > + parent.is(localStorage.getItem("persistent2"), "persistent value 2"); define var is = parent.is; at the top. it will make the changes (and the patch) simpler. ::: dom/tests/mochitest/localstorage/frameCookieSession-phase2.html @@ +12,5 @@ > + switch (location.search) { > + case '?1': > + test1(); > + return; > + break; don't need to break after return @@ +50,3 @@ > > localStorage.setItem("session only 2", "must be deleted on drop of session-only cookies permissions"); > + SpecialPowers.pushPermissions([{'type': 'cookie', 'allow': SpecialPowers.Ci.nsICookiePermission.ACCESS_DEFAULT, 'context': document}], function() { window.location.search = '?1'; }); do you really need to reload the frame? btw, with this approach you can do it all in a just a single iframe or even use the top test_....html with query strings as src for the test iframe. ::: dom/tests/mochitest/sessionstorage/test_cookieSession.html @@ +17,5 @@ > +var testFrame; > +function iframeOnload(aValue) { > + switch (aValue) { > + case 1: > + testframe.onload = test1; testFrame or testframe ?? @@ +42,5 @@ > + of(false, 'should not be reached'); > + SimpleTest.finish(); > + return; > + } > + window.frames[0].location.reload(); window.frames[0] or testframe ? this needs an explanation comment in the test. I don't get what this is supposed to do. ::: dom/tests/mochitest/sessionstorage/test_sessionStorageBaseSessionOnly.html @@ +20,5 @@ > + of(false, 'should not be reached'); > + SimpleTest.finish(); > + return; > + } > + window.frames[0].location.reload(); same comments as for test_coockieSession
Attachment #805347 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 14•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #13) > do you really need to reload the frame? Yes, otherwise the permissions changes wouldn't be picked up. I've seen it more often in b2g, where you need to reload an iframe before the permission changes get into effect. Thanks for the review, I'll be working on an updated patch.
![]() |
||
Comment 15•8 years ago
|
||
Thanks, just add more comments, explain what you do and why. The patch could already get r+ if I had the explanatory notes before ;)
Assignee | ||
Comment 16•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #13) > btw, with this approach you can do it all in a just a single iframe or even > use the top test_....html with query strings as src for the test iframe. Ok, I did that in this patch (I was actually contemplating to do that). The naming of the functions in test_cookieSession.html are a bit weird, still, perhaps. I added a comment that explains the iframe reloading: /* After every permission change, an iframe has to be reloaded, otherwise this test causes failures in b2g (oop) mochitest */ Is that enough?
Attachment #805347 -
Attachment is obsolete: true
Attachment #810233 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•8 years ago
|
Attachment #810233 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 17•8 years ago
|
||
Forgot to remove these files in Makefile.in: + frameCookieSession-phase1.html \ + frameCookieSession-phase2.html \
Attachment #810233 -
Attachment is obsolete: true
Attachment #810235 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 18•8 years ago
|
||
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bf5256b5bfff
![]() |
||
Comment 19•8 years ago
|
||
(In reply to Martijn Wargers [:mwargers] (QA) from comment #16) > I added a comment that explains the iframe reloading: > /* After every permission change, an iframe has to be reloaded, > otherwise this test causes failures in b2g (oop) mochitest */ > > Is that enough? And why exactly it fails? :)
Assignee | ||
Comment 20•8 years ago
|
||
I've now added this comment: /* After every permission change, an iframe has to be reloaded, otherwise this test causes failures in b2g (oop) mochitest, because the permission changes don't seem to be always picked up by the code that excercises it */ I don't know exactly myself why this is necessary and what exactly is failing.
Attachment #810235 -
Attachment is obsolete: true
Attachment #810235 -
Flags: review?(honzab.moz)
Attachment #810718 -
Flags: review?(honzab.moz)
![]() |
||
Updated•8 years ago
|
Attachment #810718 -
Attachment is patch: true
![]() |
||
Comment 21•8 years ago
|
||
Is there a try push?
Assignee | ||
Comment 22•8 years ago
|
||
Yes, see comment 18.
![]() |
||
Comment 23•8 years ago
|
||
Comment on attachment 810718 [details] [diff] [review] 913706_5.difff Review of attachment 810718 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! r=honzab Check the comments, but nothing critical is there. ::: dom/tests/mochitest/localstorage/test_cookieSession.html @@ +18,5 @@ > +function startTest() { > + localStorage.setItem("persistent1", "persistent value 1"); > + localStorage.setItem("persistent2", "persistent value 2"); > + > + SpecialPowers.pushPermissions([{'type': 'cookie', 'allow': SpecialPowers.Ci.nsICookiePermission.ACCESS_SESSION, 'context': document}], testa); So, last time you claimed that permissions are not propagated (or doesn't take effect) until you reload the window content, but here you are not doing it... I'm confused. @@ +94,5 @@ > +switch (location.search) { > + case '?a': > + startTest(); > + break; > + case '?b': I don't think you need to stick with the original phase1 (aka 'a') and phase2 (aka 'b'). Up to you, but you can very well be ok with just numbers for each separate sub test function (test1, test2 ... test6). And you can also just do eval(location.search.substring(1) + "()"); and pass directly the function name to get rid of the switch. Helper function to wrap the long SpecialPowers.pushPermissions on and on repeating line would be good. @@ +115,5 @@ > + var iframe = document.createElement('iframe'); > + iframe.src = 'test_cookieSession.html?a'; > + setTimeout(function() { > + // document.body is null without this timer > + document.body.appendChild(iframe); Do this in body onload ? ::: dom/tests/mochitest/sessionstorage/test_cookieSession.html @@ +14,5 @@ > storage. > */ > > +var testframe; > +function iframeOnload(aValue) { better signature would be pushPermissionAndTest(aPermission, aValue), the frame is irrelevant to the logic (it's just your hidden helper under this function). You can wrap call to SpecialPowers.pushPermissions here as well. I think you can pass the function directly via aValue and get rid of the switch. Then you can have a nice and readable code like: function test1() { ... pushPermissionAndTest(ACCESS_SESSION, test2) } function test2() ... And even best would be to have an array of functions you can just shift() off. All is up to you, tho.
Attachment #810718 -
Flags: review?(honzab.moz) → review+
![]() |
||
Comment 24•8 years ago
|
||
(In reply to Martijn Wargers [:mwargers] (QA) from comment #22) > Yes, see comment 18. It's heavily orange on OSX. Seems like some other patch of yours did that, so it should be OK. I leave this up to your responsibility.
Assignee | ||
Comment 25•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #23) > > + SpecialPowers.pushPermissions([{'type': 'cookie', 'allow': SpecialPowers.Ci.nsICookiePermission.ACCESS_SESSION, 'context': document}], testa); > > So, last time you claimed that permissions are not propagated (or doesn't > take effect) until you reload the window content, but here you are not doing > it... I'm confused. Yes, well, I only did the "reload the window" thing where it would cause failures, otherwise. For the instances like this one, it didn't cause failures, so I didn't do the "reload the window" thing. I don't know why it is sometimes necessary and sometimes not. I'll try to address your review comments for the rest.
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #24) > It's heavily orange on OSX. Seems like some other patch of yours did that, > so it should be OK. I leave this up to your responsibility. Yes, that's unrelated to this patch, I know what caused that.
Assignee | ||
Comment 27•8 years ago
|
||
> Yes, well, I only did the "reload the window" thing where it would cause > failures, otherwise. For the instances like this one, it didn't cause > failures, so I didn't do the "reload the window" thing. > I don't know why it is sometimes necessary and sometimes not. I talked with Jonas Sicking about this last week and he thinks it's a bug, so I filed bug 927208 for it.
Assignee | ||
Comment 28•8 years ago
|
||
Updated patch from bitrot and addressed review comments. > And you can also just do eval(location.search.substring(1) + "()"); and pass > directly the function name to get rid of the switch. That's the only thing I didn't do, because when running test_cookie.html as a standalone test, this would go wrong, I think. Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b3ca94603323
Attachment #810718 -
Attachment is obsolete: true
Assignee | ||
Comment 29•8 years ago
|
||
(In reply to Martijn Wargers [:mwargers] (QA) from comment #28) > Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=b3ca94603323 Ugh, I didn't mean to push those other patches in the try run, but it shouldn't matter.
Assignee | ||
Comment 30•8 years ago
|
||
Comment on attachment 8334720 [details] [diff] [review] 913706_6.diff Review of attachment 8334720 [details] [diff] [review]: ----------------------------------------------------------------- Tryserver seems to go well (the failures are coming from the offline tests, because of the accidental check-in of an unrelated patch)
Attachment #8334720 -
Flags: review?(honzab.moz)
![]() |
||
Comment 31•8 years ago
|
||
(In reply to Martijn Wargers [:mwargers] (QA) from comment #28) > > And you can also just do eval(location.search.substring(1) + "()"); and pass > > directly the function name to get rid of the switch. > > That's the only thing I didn't do, because when running test_cookie.html as > a standalone test, this would go wrong, I think. I don't know why it shouldn't. You can also do this[location.search.substring(1)](); But this is not important.
![]() |
||
Comment 32•8 years ago
|
||
Comment on attachment 8334720 [details] [diff] [review] 913706_6.diff Review of attachment 8334720 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/tests/mochitest/sessionstorage/test_cookieSession.html @@ +20,2 @@ > > +function pushPermissionAndTest(aValue, aNext) { I think the aValue, aNext args are excessive here. @@ +21,5 @@ > +function pushPermissionAndTest(aValue, aNext) { > + var test = tests.shift(); > + if (test) { > + document.getElementById('testframe').onload = test; > + /* After every permission change, an iframe has to be reloaded, ws
Attachment #8334720 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 33•8 years ago
|
||
I addressed the 2 nits.
Attachment #8334720 -
Attachment is obsolete: true
Assignee | ||
Comment 34•8 years ago
|
||
Pushed to try, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=22e413ce9255
Assignee | ||
Comment 35•8 years ago
|
||
Tryserver green, this is ready to be checked in now.
Keywords: checkin-needed
Comment 36•8 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/c381080acb6b
Flags: in-testsuite+
Keywords: checkin-needed
Comment 37•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c381080acb6b
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
You need to log in
before you can comment on or make changes to this bug.
Description
•