1054418 - cache2 automation: browser_cmd_appcache_valid.js fails with cache2 enabled

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Jason Duell]

See 'dt' test failures in 

  https://tbpl.mozilla.org/?tree=Try&rev=a1ea7549c4fd

225 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/devtools/commandline/test/browser_cmd_appcache_valid.js | html output for 'appcache clear' should match /successfully/. Actual textContent: "[Exception... "Component returned failure code: 0x80004001 (NS_ERROR_NOT_IMPLEMENTED) [nsICacheService.evictEntries]"  

Looks like we're calling EvictEntries and it's not implemented.

Comment 1

[Steve Workman [:sworkman] (INACTIVE)]

Taking a look at this now. Looks like we could rewrite AppCacheUtils.clearAll to use Services.cache2 instead of .cache.

Let me see what I can do...

Comment 2

[Steve Workman [:sworkman] (INACTIVE)]

I have a fix that seems to work locally. So, let's see what happens on try. (Inserted a patch to enable cache2 on tests first).

https://tbpl.mozilla.org/?tree=Try&rev=3ad353d01879
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3ad353d01879

Comment 3

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Rewrite AppCacheUtils.jsm to use HTTP Cache v2 APIs

Comment 4

[Jason Duell]

Be careful--IIRC the appcache is still using the cache1 APIs (i.e. it's actually still using the old cache).  We removed the IDLs for the old cache, but added internal APIs for accessing it. See bug 999577 (look at nsCacheService.h/cpp).  So I'm guessing you just need a call to EvictEntriesInternal()?

Comment 5

[Jason Duell]

Ah, I see this is JS code.  Hmm.  We might need to re-expose the nsICacheService.evictEntries function somehow (maybe give it a deprecated-sounding name like evictEntriesDeprecated()) if I'm right about needing to use the v1 code here.

Comment 6

[Jason Duell]

Sounds like it's worth a grep through the tree for old uses of the nsCacheService IDLs, in case we missed any others.

Comment 7

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Jason Duell (:jduell) from comment #5)
> Ah, I see this is JS code.  Hmm.  We might need to re-expose the
> nsICacheService.evictEntries function somehow (maybe give it a
> deprecated-sounding name like evictEntriesDeprecated()) if I'm right about
> needing to use the v1 code here.

Unsure. Looks like the v2 api I used uses internal v1 apis anyway.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Jason Duell (:jduell) from comment #6)
> Sounds like it's worth a grep through the tree for old uses of the
> nsCacheService IDLs, in case we missed any others.

Did a little bit of pruning in bug 1054572. Mostly redundant references that can be cleaned up. Excludes FTP code which needs more attention and is covered in bug 913827.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8473921 [details] [diff] [review]
v1.0 Rewrite AppCacheUtils.jsm to use HTTP Cache v2 APIs

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

::: browser/devtools/shared/AppCacheUtils.jsm
@@ +298,5 @@
> +    }
> +
> +    let appCacheStorage = Services.cache2.appCacheStorage(LoadContextInfo.default, null);
> +    appCacheStorage.asyncEvictStorage({
> +      onCacheEntryDoomed: function(result) {}

you can pass null as the callback

Comment 10

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Michal!

https://hg.mozilla.org/integration/mozilla-inbound/rev/8474576a30f0

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8474576a30f0

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Please nominate this for Aurora/Beta approval :)

Comment 13

[Jason Duell]

Comment on attachment 8473921 [details] [diff] [review]
v1.0 Rewrite AppCacheUtils.jsm to use HTTP Cache v2 APIs

Approval Request Comment
[Feature/regressing bug #]: cache2
[User impact if declined]: cache2 doesn't pass automated tests
[Describe test coverage new/current, TBPL]: existing browser_cmd_appcache_valid.js showed the bug (and the fix)
[Risks and why]: test was using deprecated API: low risk
[String/UUID change made/needed]: none

Comment 14

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Don't mark cache entries as classified if on the allow list

This should help fix the mochitest failure in test_classified_annotations.html. Turns out that nsChannelClassifier was still using nsICacheEntryDescriptor from cache v1; so it wasn't writing or reading any cache entry metadata. Seems like nsChannelClassifier is not dependent on the cache, but we should still land this asap on m-c and try to land on aurora and beta.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b2a5dbdc4e01
https://hg.mozilla.org/releases/mozilla-beta/rev/fa7360fe9779