1250331 - Get basic appcache tests working on e10s

Status: RESOLVED WONTFIX

(Core :: Networking: Cache, defect, P5)

Description

[Blake Kaplan (:mrbkap) (inactive)]

There are 29 tests in the dom/tests/mochitest/ajax/offline directory dealing with appcache. They are all currently disabled for e10s (and b2g!), so we don't have any real visibility into how correct the code is for e10s. I have heard rumors that we want to remove appcache in favor of service workers but that hasn't happened yet.

We need to decide what to do with the mochitests. I'm not sure who owns appcache, so Patrick, I'll start with you: Do we care about these tests in e10s? If we don't, we can mark them as "ignore" on the spreadsheet found at [1]. If we do, then we'll have to make them e10s compatible, which will probably mean using a chrome script to observe the proper notifications in the parent and send the right information down to the child. I can help with this, if needed.

(One thing that might be a saving grace if we need to make them e10s compatible: they appear to mostly use a common offlineTests.js structure that could possibly fix most or all of the tests in one fell swoop).

[1] https://docs.google.com/spreadsheets/d/10UeyRoiWV2HjkWwAU51HXyXAV7YLi4BjDm55mr5Xv6c/edit#gid=368835050

Comment 1

[:Felipe Gomes (needinfo for replies!)]

oh, coincidence, I just sent an e-mail to Honza today asking about these tests, and he said we should probably not bother with them.. But I'd like to see what Patrick says, because as I understand there's a long process to deprecate this feature

Comment 2

[Patrick McManus [:mcmanus]]

I've marked them ignore. thanks!

1] appcache is going away in the medium term
2] appcache is pretty low risk for regression because I wontfix any non critical changes near it due to 1..
3] rule 2 sort of applies here too.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

It appears that this decision was revisited. I've unmarked the appcache tests as "ignore" and I'm reopening this bug.

Comment 4

[Honza Bambas (:mayhemer)]

I've quickly looked at the tests to assess how much work it could be to fix them.

There are two problems (so far discovered):
- some tests are calling on windows.applicationCache.mozItems et al content API that is not implemented in content (because it would mean complicated IPC sync calls to the parent) ; these all are not part of the appcache spec (used to be, but were loooong ago removed), are moz- prefixed (this firefox specifi) and probably not widely if at all used => I vote for that API no longer being tested and simply put as "known to not work" in e10s

- the root test js helper (offlineTests.js script) calls on nsIApplicationCacheService intensively (from content) ; that is a gateway to the cache service (when on the parent process.)  don't we happen to have some mechanism to do the IPC calls already for mochitests?  if so, it could be easy to update offlienTests.js with it


There may be more problems, but I would have to check the tests one by one which is pretty time consuming.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> - the root test js helper (offlineTests.js script) calls on
> nsIApplicationCacheService intensively (from content) ; that is a gateway to
> the cache service (when on the parent process.)  don't we happen to have
> some mechanism to do the IPC calls already for mochitests?  if so, it could
> be easy to update offlienTests.js with it

We do! You can use SpecialPowers.loadChromeScript to load a script in the parent process (or in the same process in non-e10s). https://wiki.mozilla.org/Electrolysis/e10s_test_tips#Mochitest_.28plain.29 has some more info. You can often set observers in a chrome script and a message listener on your chrome script in the test and use the message listener instead of the observer service.

> There may be more problems, but I would have to check the tests one by one
> which is pretty time consuming.

This sounds like a good start.

Comment 6

[Honza Bambas (:mayhemer)]

Thanks for the info, Blake!

Only these functions need to be converted to be run on parent:

OfflineTest.checkCache
  - must as a whole run on the parent (takes a callback, can be async)
OfflineTest.getActiveCache
  - must examine nsIApplicationCacheService on the parent with manifest URL carried from child ; synchronously returns a fake object (or null when no cache is present for the manifest) with at least discard() method on it that will have to by capable of another sync call to parent (nsIApplicationCache.discard())

OfflineTest.waitForAdd will be removed.

We have to add synchronous OfflineTest.chooseApplicationCache that on the parent calls on nsIApplicationCacheService.chooseApplicationCache.


Any volunteers ;) ?

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: wip to start on

- some basic simplifications
- remove tests for mozItems et al from (so far only) simpleManifest test

Comment 9

[Jim Mathies [:jimm]]

don't fix these, we're going to kill appcache off now.

Comment 10

[Honza Bambas (:mayhemer)]

(In reply to Jim Mathies [:jimm] from comment #9)
> don't fix these, we're going to kill appcache off now.

\   /
 \O/

Comment 11

[Honza Bambas (:mayhemer)]

Based on further discussion (which is like April weather! :)) reopening.  We will try to find someone to work on this according the "wip to start on" patch and comment 4 (mainly removing the dynamic items support) + comment 6 instructions.

Comment 12

[Honza Bambas (:mayhemer)]

Also, you may use following functions:

https://dxr.mozilla.org/mozilla-central/rev/9bd90088875399347b05d87c67d3709e31539dcd/testing/specialpowers/content/specialpowersAPI.js#1473

https://wiki.mozilla.org/Electrolysis/e10s_test_tips#Mochitest_.28plain.29 (mainly the services part)

Comment 13

[Jason Duell]

Not sure what the needinfo is for exactly, but as comment 11 notes, we are re-opening this and will have some basic e10s appcache tests.  Not sure yet if that'll be honza who does it or hopefully someone else I find who he can mentor,

Comment 14

[Jason Duell]

We've done hand-testing of appcache on e10s, and the code is unlikely to change often enough (hopefully it won't change at all before we eventually remove it).  Automating e10s tests is looking like more work than it's worth--we can just hand-test again if we change the code.

I'd vote for e10s-untracking this.

Comment 15

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Appcache was removed.