Fix and enable offline cache mochitests

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: ferjm, Assigned: emk)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(4 attachments, 11 obsolete attachments)

15.63 KB, patch
Details | Diff | Splinter Review
5.31 KB, patch
Details | Diff | Splinter Review
15.77 KB, patch
Details | Diff | Splinter Review
59 bytes, text/x-review-board-request
mayhemer
: review+
Details
Blocks: 781696
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
Assignee: nobody → martijn.martijn
Created attachment 788731 [details] [diff] [review]
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.
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
Created attachment 789155 [details] [diff] [review]
871323.diff

I'll see how this goes on my local b2g build.
Attachment #788731 - Attachment is obsolete: true
Created attachment 789170 [details] [diff] [review]
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
Attachment #789155 - Attachment is obsolete: true
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

5 years ago
Honza would know for sure, but yes, I suspect you need do to this on the chrome process.
Comment on attachment 789170 [details] [diff] [review]
871323.diff

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

Try server was green.
Attachment #789170 - Flags: review?(jgriffin)
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 on attachment 789170 [details] [diff] [review]
871323.diff

Looks like this is a no-go from honza's feedback.
Attachment #789170 - Flags: review?(jgriffin) → review-
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.
Attachment #789170 - Flags: review- → review+
Created attachment 789805 [details] [diff] [review]
871323.diff (for check-in)
Attachment #789170 - Attachment is obsolete: true
Please leave bug open after check-in.
Keywords: checkin-needed
Whiteboard: [leave-open]
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.
Keywords: checkin-needed
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.
(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 :)
Thanks for the offer, no, you don't have to re-merge the patch.
The fix for bug 914633 might help here in rewriting the tests.
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.
[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
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 :)
Flags: needinfo?(jonas)
Created attachment 813877 [details] [diff] [review]
871323_a.diff

This removes most of the enablePrivilege calls and uses pushPermissions.
Attachment #789805 - Attachment is obsolete: true
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.
Depends on: 918880
Created attachment 815911 [details] [diff] [review]
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.
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?
Flags: needinfo?(jonas)
(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?
Flags: needinfo?(jonas)
(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.
Yes, please add if() checks rather than remove the tests completely.
Flags: needinfo?(jonas)
Comment on attachment 813877 [details] [diff] [review]
871323_a.diff

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

This is just some enablePrivilege cleanup.
Attachment #813877 - Flags: review?(honzab.moz)
Created attachment 818154 [details] [diff] [review]
871323.diff (wip patch)

Sorry, the previous patch had some unrelated cruft, this is the correct wip patch.
Attachment #815911 - Attachment is obsolete: true
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.
Attachment #813877 - Flags: review?(honzab.moz) → review-
Created attachment 8334755 [details] [diff] [review]
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.
Attachment #813877 - Attachment is obsolete: true
Attachment #8334755 - Flags: review?(honzab.moz)
Created attachment 8334824 [details] [diff] [review]
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 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
Attachment #8334755 - Flags: review?(honzab.moz) → review+
Created attachment 8342614 [details] [diff] [review]
871323_a.diff (checked in)

I addressed the last nit.
Attachment #8334755 - Attachment is obsolete: true
Created attachment 8342683 [details] [diff] [review]
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.
Attachment #8342683 - Flags: review?(honzab.moz)
Attachment #8342683 - Flags: review?(honzab.moz) → review+
Created attachment 8343038 [details] [diff] [review]
871323_b.diff (checked in)

871323_a.diff and 871323_b.diff can be checked in now, they were green on tryserver.
Attachment #818154 - Attachment is obsolete: true
Attachment #8334824 - Attachment is obsolete: true
Attachment #8342683 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/integration/b2g-inbound/rev/21c6774bb077

I folded the two patches.
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [leave-open]
Was it on purpose that b2g.json wasn't updated?
Flags: needinfo?(martijn.martijn)
Whiteboard: [leave open]
There is no b2g.json in the patch, these 2 patches didn't fix anything for b2g yet. That will come later.
Flags: needinfo?(martijn.martijn)
Attachment #8342614 - Attachment description: 871323_a.diff (for check-in) → 871323_a.diff (checked in)
Attachment #8343038 - Attachment description: 871323_b.diff (for check-in) → 871323_b.diff (checked in)
Martijn - The dependent bug is fixed here. Are we able to finish this off now to get these tests turned on?
Flags: needinfo?(martijn.martijn)
Yes, this should be entirely fixable now.
Flags: needinfo?(martijn.martijn)
Created attachment 8356501 [details] [diff] [review]
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.
Ok, it seems the problem comes from test_bug460353.html.
Apparently, there is something with the changes I made related to there.
Created attachment 8357356 [details] [diff] [review]
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.
Attachment #8356501 - Attachment is obsolete: true
Duplicate of this bug: 645695
Blocks: 462483
Blocks: 984012
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Patrick, why was this marked WONTFIX?
Flags: needinfo?(mcmanus)
we'll be removing appcache when service workers are up on their feet. so only taking critical fixes for it in the interim.
Flags: needinfo?(mcmanus)
Ah, I see, service workers is going to replace app cache.
Assignee: martijn.martijn → nobody
(Assignee)

Comment 51

2 years ago
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")
Flags: needinfo?(mcmanus)
Flags: needinfo?(mcmanus) → needinfo?(jduell.mcbugs)

Comment 52

2 years ago
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).
Flags: needinfo?(jduell.mcbugs) → needinfo?(honzab.moz)
(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.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 54

2 years ago
OK, thank you.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7c7d7fcd2d33586c5396d62ee242fc69797d9525
Assignee: nobody → VYV03354
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Summary: Fix and enable offline cache mochitests for B2G → Fix and enable offline cache mochitests
Comment hidden (mozreview-request)
Whiteboard: [leave open] → [leave open][necko-active]
(Assignee)

Updated

2 years ago
Whiteboard: [leave open][necko-active] → [necko-active]
(Assignee)

Updated

2 years ago
OS: Gonk (Firefox OS) → All
Hardware: ARM → All

Comment 56

2 years ago
mozreview-review
Comment on attachment 8830918 [details]
Bug 871323 - Remove enablePrivilege from offline tests.

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

nice
Attachment #8830918 - Flags: review?(honzab.moz) → review+

Comment 57

2 years ago
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/6c25418fa549
Remove enablePrivilege from offline tests. r=mayhemer

Comment 58

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6c25418fa549
Status: REOPENED → RESOLVED
Last Resolved: 3 years ago2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.