871323 - Fix and enable offline cache mochitests

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

Offline cache mochitests are failing and disabled for b2g https://hg.mozilla.org/mozilla-central/file/4ff8aa4a7295/testing/mochitest/b2g.json#l213

Comment 1

[Martijn Wargers (dead)]

There are a whole bunch of failures like this:
 uncaught exception - NS_ERROR_NOT_AVAILABLE: Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsIPermissionManager.addFromPrincipal] at http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/offlineTests.js:114

I'll move offlineTests.js to use SpecialPowers and then we can see how much that improves things.
http://mxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/ajax/offline/offlineTests.js

Comment 2

[Martijn Wargers (dead)]

Attachment: offlineTests.js

This is wip. I'm not able to remove all the enablePrivilege calls without getting errors.
I think all the offline tests needs fixing and their enablePrivilege calls removed.

Comment 3

[Martijn Wargers (dead)]

Actually, for b2g mochitests, it might make it work, but there are some more offline test files that use permission setting wrong:
dom/tests/mochitest/ajax/offline/test_missingManifest.html 
dom/tests/mochitest/ajax/offline/test_obsolete.html 
dom/tests/mochitest/ajax/offline/test_bug460353.html

Comment 4

[Martijn Wargers (dead)]

Attachment: 871323.diff

I'll see how this goes on my local b2g build.

Comment 5

[Martijn Wargers (dead)]

Attachment: 871323.diff

Ok, this doesn't fix it for b2g, but this is still a good cleanup.
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=61574e12632c

Comment 6

[Martijn Wargers (dead)]

After that patch, when trying out these tests on b2g, I get:
JavaScript error: chrome://specialpowers/content/specialpowersAPI.js, line 88: NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIApplicationCacheService.getActiveCache]
and:
JavaScript error: http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/test_bug744719.html, line 16: NS_ERROR_NOT_IMPLEMENTED: Method not implemented
for instance.

So I guess perhaps this stuff that touches nsIApplicationCacheService, etc, has to be done in the chrome process (by forwarding it to the specialpowersObserver)?

Comment 7

[Jason Duell]

Honza would know for sure, but yes, I suspect you need do to this on the chrome process.

Comment 8

[Martijn Wargers (dead)]

Comment on attachment 789170 [details] [diff] [review]
871323.diff

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

Try server was green.

Comment 9

[Honza Bambas (:mayhemer)]

I believe that the really correct solution here will be a bit harder.

Problem is that managing appcaches (i.e. call on nsIApplicationCache* et al - those are the internal classes to manager appcaches, not the objects exposed to DOM content) must be done on the parent (chrome) process.  Those need access to profile and work only on the parent process.  That is why Martijn sees the NOT_IMPLEMENTED error.

However, checking pages load from appcache must happen on the content process.  That is where it happens during normal usage.

So it's quite complicated to run these test in multiprocess environment.  We may think of some kind of an IPC or whatever framework to make this simpler.  Most of the tests should actually work the following way:
- on the parent process setup privileges
- on the content process check the cache behaves (load a page, register event listeners, etc)
- on the parent process check appcache content (check stuff is actually cached / or not cached) ; change appcache status, do what ever privileged changes needed to continue the test
- on the content process do next step of the test like reloading a page etc.
- on the parent process check the new status of the cache
- etc...
- finally, on the parent process do a complete cleanup (delete app cache, remove privileges)

Comment 10

[Jonathan Griffin (:jgriffin)]

Comment on attachment 789170 [details] [diff] [review]
871323.diff

Looks like this is a no-go from honza's feedback.

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 789170 [details] [diff] [review]
871323.diff

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

Ah, sorry, didn't realize this was only SpecialPowers-related cleanup.

Comment 12

[Martijn Wargers (dead)]

Attachment: 871323.diff (for check-in)

Comment 13

[Martijn Wargers (dead)]

Please leave bug open after check-in.

Comment 14

[Honza Bambas (:mayhemer)]

Can we please wait with landing this?  I'm about to land a relatively complex change to offline tests right now.  This would make me have a hard time with merging.

I'll help merge this patch on top of my changes then.

Thanks for understanding.

Comment 15

[Martijn Wargers (dead)]

I guess that was bug 892488.
The patch would have to be rewritten, it's not that important, because it doesn't fix the tests for b2g and eventually the suggestion in comment 9 has to be done.

Comment 16

[Honza Bambas (:mayhemer)]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #15)
> I guess that was bug 892488.
> The patch would have to be rewritten, it's not that important, because it
> doesn't fix the tests for b2g and eventually the suggestion in comment 9 has
> to be done.

If you don't want me to re-merge your patches, please let me know.  I can save some time :)

Comment 17

[Martijn Wargers (dead)]

Thanks for the offer, no, you don't have to re-merge the patch.

Comment 18

[Martijn Wargers (dead)]

The fix for bug 914633 might help here in rewriting the tests.

Comment 19

[Martijn Wargers (dead)]

I looked at this a bit more, specifically test_bug744719.html. This triggers:
JavaScript error: http://mochi.test:8888/tests/dom/tests/mochitest/ajax/offline/test_bug744719.html, line 16: NS_ERROR_NOT_IMPLEMENTED: Method not implemented
for instance.

This points to:
ok(applicationCache.mozItems.length == 0,
..which exercises this code:
http://mxr.mozilla.org/mozilla-central/source/dom/src/offline/nsDOMOfflineResourceList.cpp#187
189   if (IS_CHILD_PROCESS()) 
190     return NS_ERROR_NOT_IMPLEMENTED;

And most of the methods of applicationCache seem to have this. So these mochitests that test offline cache can't be run from the child process currently.

Comment 20

[Martijn Wargers (dead)]

[22:28]	<mwargers>	mayhemer: hey, I was looking at bug 871323, it seems that a lot of the window.applicationCache methods don't work in child processes
[22:29]	<mayhemer>	mwargers: few, IIRC only the moz- prefixed ones
[22:29]	<mwargers>	mayhemer, ah, ok
[22:30]	<mayhemer>	mwargers: I don't think that is a good idea to deal it with it
[22:30]	<mwargers>	mayhemer, so how should I handle that in b2g mochitest, which is run in child process? Add some check that b2g is running when that happens?
[22:30]	<firebot>	Check-in: http://hg.mozilla.org/integration/mozilla-inbound/rev/784b1e8abd5b - Ethan Hugg - Bug 916429 - use sctpmap line for datachannels r=jesup
[22:31]	<mayhemer>	mwargers: are any of these unimlmented methods exercised? I think some of them are...
[22:31]	<mayhemer>	mwargers: if those are really only moz- prefixed ones that cause problems, then remove them from existing tests
[22:32]	<mwargers>	mayhemer, remove them? But then the moz-prefixed methods are not tested at all anymore
[22:33]	<mayhemer>	mwargers: I think those are, but check on that
[22:33]	<mayhemer>	mwargers: ah
[22:33]	<mayhemer>	mwargers: I don't care :) no one is using them
[22:34]	<mwargers>	mayhemer, ok then
[22:34]	<mayhemer>	mwargers: those are undocumented, causing perf issues, and will be obsolete when we have the NavigationControler done
[22:34]	<mayhemer>	mwargers: maybe ask jonas about it first

Comment 21

[Honza Bambas (:mayhemer)]

The question is: should mozXXX prefixed applicationCache content object functions be removed from test coverage because they are untestible (unimplemented) for child process?

Now we test some of them (mozAdd and mozLength I think).  Those are prefixed all the time this API is public, cause perf issues (mainthread IO), and IMO are not used at all and will be altered with NavigationControler.

To remove them from test coverage would make martijn's life simpler :)

Comment 22

[Martijn Wargers (dead)]

Attachment: 871323_a.diff

This removes most of the enablePrivilege calls and uses pushPermissions.

Comment 23

[Martijn Wargers (dead)]

Ok, from bug 767258, comment 9, when offline-apps.allow_by_default=true, the offline-app permissions is automatically set for that page.
However, that is not working in multi process, hence I get permission issues there.

I should be able to just add these offline-app permissions for every offline test in the test themselves.
But it makes me wonder why the offline-app permission is being set in the OfflineTest.setup function at all, that seems superfluous to me.

Comment 24

[Martijn Wargers (dead)]

Attachment: 871323.diff

Sorry, here is the promised wip patch that I'm working on. I thought I would be able to get something that's working completely correct, but I'm not sure if I'll have the time until next week.
Problem with this patch, it's reporting wrong amount of results for test_bug744719.html. I know where that problem is coming from, I haven't been able to fix it yet, though. 
This wip patch is rather messy, because it's a lot of trial and error work, basically.

For multi-process, I probably have to convert some more stuff (recently, only tested it in single-process). For multi-process, this depends on the patch for bug 767258, because permissions are not set correctly, otherwise.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

If some appcache features aren't yet implemented in child processes then I think it's ok to remove test coverage for those for B2G. We are probably not going to spend time implementing those features in childprocesses.

Does that answer the question?

Comment 26

[Honza Bambas (:mayhemer)]

(In reply to Jonas Sicking (:sicking) Please ni? if you want feedback from comment #25)
> If some appcache features aren't yet implemented in child processes then I
> think it's ok to remove test coverage for those for B2G. We are probably not
> going to spend time implementing those features in childprocesses.
> 
> Does that answer the question?

We want to have the same set of tests for single process and e10s.  So, if we remove it for e10s, we remove it from single process testing too.

The question was: is it ok to remove test coverage for e10s-unimplemeted features all together?

Comment 27

[Martijn Wargers (dead)]

(In reply to Honza Bambas (:mayhemer) from comment #26)
> The question was: is it ok to remove test coverage for e10s-unimplemeted
> features all together?

I can add checks like "if (SpecialPowers.isMainProcess())" to test it only in single process browsers. This I in fact did in the wip patch.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, please add if() checks rather than remove the tests completely.

Comment 29

[Martijn Wargers (dead)]

Comment on attachment 813877 [details] [diff] [review]
871323_a.diff

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

This is just some enablePrivilege cleanup.

Comment 30

[Martijn Wargers (dead)]

Attachment: 871323.diff (wip patch)

Sorry, the previous patch had some unrelated cruft, this is the correct wip patch.

Comment 31

[Honza Bambas (:mayhemer)]

Comment on attachment 813877 [details] [diff] [review]
871323_a.diff

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

r-

I don't think test_bug460353.html change are correct

::: dom/tests/mochitest/ajax/offline/offlineTests.js
@@ +348,2 @@
>      func(arguments);
>    }

thinking if this "priv" hack is still needed. IIRC this was introduced to provide privileged context to the function, but apparently this has changed.

::: dom/tests/mochitest/ajax/offline/test_bug460353.html
@@ +31,2 @@
>    applicationCache.oncached = onUpdatePassed;
>    applicationCache.onnoupdate = onUpdatePassed;

this must be set before body.onload

just put it out a function

there might be no need for pushPerm callback at all.. if it ensures the permission is set synchronously on the content process (before onload).

::: dom/tests/mochitest/ajax/offline/test_obsolete.html
@@ +8,5 @@
>  <script type="text/javascript">
>  
>  var gTestWin;
>  
> +SimpleTest.waitForExplicitFinish();

better at the end of the script, but up to you.

Comment 32

[Martijn Wargers (dead)]

Attachment: 871323_a.diff

(In reply to Honza Bambas (:mayhemer) from comment #31)
> ::: dom/tests/mochitest/ajax/offline/offlineTests.js
> @@ +348,2 @@
> >      func(arguments);
> >    }
> 
> thinking if this "priv" hack is still needed. IIRC this was introduced to
> provide privileged context to the function, but apparently this has changed.

Up to you, but it probably isn't that important now, because more changes to this files is coming anyway, probably.

> ::: dom/tests/mochitest/ajax/offline/test_bug460353.html
> there might be no need for pushPerm callback at all.. if it ensures the
> permission is set synchronously on the content process (before onload).

There is never a guarantee for that. In fact, your comment made me realize I have to let the iframes load from inside the init() function to make sure the offline-app permission is set before the iframes have loaded.

I followed the rest of your review comments for this patch.

Comment 33

[Martijn Wargers (dead)]

Attachment: 871323.diff part 2 (wip)

This is a wip for part 2, I'm trying to split this work into multiple patches, it should be more easy to maintain that way and also easier to review.

Comment 34

[Honza Bambas (:mayhemer)]

Comment on attachment 8334755 [details] [diff] [review]
871323_a.diff

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

::: dom/tests/mochitest/ajax/offline/test_bug460353.html
@@ +33,5 @@
>  {
> + var iframes = document.getElementsByTagName('iframe');
> + iframes[0].src = "460353_iframe_nomanifest.html";
> + iframes[1].src = "460353_iframe_ownmanifest.html";
> + iframes[2].src = "460353_iframe_samemanifest.html";

indent 2 spaces

Comment 35

[Martijn Wargers (dead)]

Attachment: 871323_a.diff (checked in)

I addressed the last nit.

Comment 36

[Martijn Wargers (dead)]

Attachment: 871323_b.diff

This is a small follow-up patch, which has the useful parts of the wip patch in it, that can be reviewed and checked in. 
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=db2cc4a9d868

The rest of the wip patches can't be checked in, but they can be used as a reference to get a useful patch for the tests.

Comment 37

[Martijn Wargers (dead)]

Attachment: 871323_b.diff (checked in)

871323_a.diff and 871323_b.diff can be checked in now, they were green on tryserver.

Comment 38

[Ryan VanderMeulen [:RyanVM]]

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

I folded the two patches.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

Was it on purpose that b2g.json wasn't updated?

Comment 40

[Martijn Wargers (dead)]

There is no b2g.json in the patch, these 2 patches didn't fix anything for b2g yet. That will come later.

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/21c6774bb077

Comment 42

[Jason Smith [:jsmith]]

Martijn - The dependent bug is fixed here. Are we able to finish this off now to get these tests turned on?

Comment 43

[Martijn Wargers (dead)]

Yes, this should be entirely fixable now.

Comment 44

[Martijn Wargers (dead)]

Attachment: 871323_c.diff (wip)

This is a wip patch of the follow-up patch that I'm trying to write.
The problem is that when running: ./mach mochitest-plain  dom/tests/mochitest/ajax/offline/
It causes it to hang at test_updateCheck.html

I need to figure out why.

Comment 45

[Martijn Wargers (dead)]

Ok, it seems the problem comes from test_bug460353.html.
Apparently, there is something with the changes I made related to there.

Comment 46

[Martijn Wargers (dead)]

Attachment: 871323_c.diff (wip)

Ok, I just made a stupid error somewhere. *Phew*
Ok, I'll clean this patch up, then ask for review for it.
The resulting patch itself would be a wip, because offlineTestsChromeScript.js would still have some bad code in itself and offlineTests.js is only partly converted. But I want to make absolutely sure that it keeps working correctly in non-oop case af foremost.

Comment 48

[Martijn Wargers (dead)]

Patrick, why was this marked WONTFIX?

Comment 49

[Patrick McManus [:mcmanus]]

we'll be removing appcache when service workers are up on their feet. so only taking critical fixes for it in the interim.

Comment 50

[Martijn Wargers (dead)]

Ah, I see, service workers is going to replace app cache.

Comment 51

[Masatoshi Kimura [:emk]]

We still need to fix this to kill enablePrivilege.

Patrick, this is the last blocker of bug 462483. When appcache will be removed? If it will not be removed soon, will you accept a patch? If not, can I just disable tests? If not, how can I go forward with bug 462483? (except "wait forever until appcache is dead")

Comment 52

[Jason Duell]

emk,

We still don't know when we'll be able to remove the appcache--last I heard Microsoft office online and some IBM cloud software were still using it, and we were waiting for them to migrate off of it.

Honza, what's your take on this now?  Should we just run our appcache tests in non-e10s mode so we can get rid of enablePrivilege in mochitests? (see comment 51).  That's not pretty, but I don't know what else to suggest (other than "wait till the appcache gets removed": which I suppose is an option if we think appcache test coverage is more important).

Comment 53

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell [:jduell] (needinfo me) from comment #52)
> emk,
> 
> We still don't know when we'll be able to remove the appcache--last I heard
> Microsoft office online and some IBM cloud software were still using it, and
> we were waiting for them to migrate off of it.
> 
> Honza, what's your take on this now?  Should we just run our appcache tests
> in non-e10s mode so we can get rid of enablePrivilege in mochitests? 

enablePrivilege is e10s specific?  no.  appcache tests are not running in e10s at all.  

What we need is to convert use of plain Cc/Ci to SpecialPowers.  Then we can remove enablePrivilege.

> (see
> comment 51).  That's not pretty, but I don't know what else to suggest
> (other than "wait till the appcache gets removed": which I suppose is an
> option if we think appcache test coverage is more important).

Definitely some coverage is needed.  And non-e10s only is enough, IMO.  There are also tests performed by hand in e10s.

Comment 54

[Masatoshi Kimura [:emk]]

OK, thank you.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c7d7fcd2d33586c5396d62ee242fc69797d9525

Comment 55

[Masatoshi Kimura [:emk]]

Attachment: Bug 871323 - Remove enablePrivilege from offline tests.

Review commit: https://reviewboard.mozilla.org/r/107584/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107584/

Comment 56

[Honza Bambas (:mayhemer)]

Comment on attachment 8830918 [details]
Bug 871323 - Remove enablePrivilege from offline tests.

https://reviewboard.mozilla.org/r/107584/#review109342

nice

Comment 57

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6c25418fa549
Remove enablePrivilege from offline tests. r=mayhemer

Comment 58

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6c25418fa549