Closed Bug 913706 Opened 6 years ago Closed 6 years ago

Fix the tests for B2G mochitest that use nsICookiePermission.setAccess

Categories

(Testing :: Mochitest, defect)

x86
macOS
defect
Not set

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)

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); ?
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?
Yeah, I can take it.
Assignee: nobody → martijn.martijn
Attached patch 913706.diff (obsolete) — Splinter Review
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)
(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 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+
Attached patch 913706.diff (obsolete) — Splinter Review
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)
Attachment #802646 - Flags: review?(honzab.moz)
Attached patch 913706_4.diff (obsolete) — Splinter Review
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
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)
Attached patch 913706_5.diff (obsolete) — Splinter Review
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)
Attached patch 913706_5.diff (obsolete) — Splinter Review
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)
Tryserver is green.
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-
(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.
Thanks, just add more comments, explain what you do and why.  The patch could already get r+ if I had the explanatory notes before ;)
Attached patch 913706_5.diff (obsolete) — Splinter Review
(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)
Attachment #810233 - Flags: review?(honzab.moz)
Attached patch 913706_5.diff (obsolete) — Splinter Review
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)
(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? :)
Attached patch 913706_5.difff (obsolete) — Splinter Review
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)
Attachment #810718 - Attachment is patch: true
Is there a try push?
Yes, see comment 18.
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+
(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.
(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.
(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.
> 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.
Attached patch 913706_6.diff (obsolete) — Splinter Review
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
(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.
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)
(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 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+
Attached patch 913706_6.diffSplinter Review
I addressed the 2 nits.
Attachment #8334720 - Attachment is obsolete: true
Pushed to try, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=22e413ce9255
Tryserver green, this is ready to be checked in now.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c381080acb6b
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Depends on: 960760
You need to log in before you can comment on or make changes to this bug.