913706 - Fix the tests for B2G mochitest that use nsICookiePermission.setAccess

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Martijn Wargers (zombie)]

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

[Honza Bambas (:mayhemer)]

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?

Comment 2

[Martijn Wargers (zombie)]

Yeah, I can take it.

Comment 3

[Martijn Wargers (zombie)]

Attachment: 913706.diff

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.

Comment 4

[Honza Bambas (:mayhemer)]

(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

[Honza Bambas (:mayhemer)]

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?

Comment 6

[Martijn Wargers (zombie)]

Attachment: 913706.diff

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.

Comment 7

[Martijn Wargers (zombie)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=062aca05a6b1

Comment 8

[Martijn Wargers (zombie)]

Attachment: 913706_4.diff

Something went wrong, this patch should be ok now. Pushed this to try: https://tbpl.mozilla.org/?tree=Try&rev=d5bafdf349bb

Comment 9

[Martijn Wargers (zombie)]

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.

Comment 10

[Martijn Wargers (zombie)]

Attachment: 913706_5.diff

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.

Comment 11

[Martijn Wargers (zombie)]

Attachment: 913706_5.diff

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

Comment 12

[Martijn Wargers (zombie)]

Tryserver is green.

Comment 13

[Honza Bambas (:mayhemer)]

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

Comment 14

[Martijn Wargers (zombie)]

(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

[Honza Bambas (:mayhemer)]

Thanks, just add more comments, explain what you do and why.  The patch could already get r+ if I had the explanatory notes before ;)

Comment 16

[Martijn Wargers (zombie)]

Attachment: 913706_5.diff

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

Comment 17

[Martijn Wargers (zombie)]

Attachment: 913706_5.diff

Forgot to remove these files in Makefile.in:
+    frameCookieSession-phase1.html \
+    frameCookieSession-phase2.html \

Comment 18

[Martijn Wargers (zombie)]

Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=bf5256b5bfff

Comment 19

[Honza Bambas (:mayhemer)]

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

Comment 20

[Martijn Wargers (zombie)]

Attachment: 913706_5.difff

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.

Comment 21

[Honza Bambas (:mayhemer)]

Is there a try push?

Comment 22

[Martijn Wargers (zombie)]

Yes, see comment 18.

Comment 23

[Honza Bambas (:mayhemer)]

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.

Comment 24

[Honza Bambas (:mayhemer)]

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

Comment 25

[Martijn Wargers (zombie)]

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

Comment 26

[Martijn Wargers (zombie)]

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

Comment 27

[Martijn Wargers (zombie)]

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

Comment 28

[Martijn Wargers (zombie)]

Attachment: 913706_6.diff

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

Comment 29

[Martijn Wargers (zombie)]

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

[Martijn Wargers (zombie)]

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)

Comment 31

[Honza Bambas (:mayhemer)]

(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

[Honza Bambas (:mayhemer)]

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

Comment 33

[Martijn Wargers (zombie)]

Attachment: 913706_6.diff

I addressed the 2 nits.

Comment 34

[Martijn Wargers (zombie)]

Pushed to try, just to be sure: https://tbpl.mozilla.org/?tree=Try&rev=22e413ce9255

Comment 35

[Martijn Wargers (zombie)]

Tryserver green, this is ready to be checked in now.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c381080acb6b

Comment 37

[Carsten Book [:Tomcat]]

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