822121 - Test failure "There is no disk storage in use - '2101248' should equal '0'" in testPrivateBrowsing/testAboutCache.js

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Andreea Matei [:AndreeaMatei]]

This happens with the new PB mode in Nightly, over platforms. The thing is the disk storage is different from a failure to another, seems the evictEntries() doesn't work as expected now.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Is that a regression introduced with the new pb mode? Ehsan or Josh, something you could already comment on?

Comment 2

[Josh Matthews [:jdm]]

With per-window private browsing enabled, the disk cache is allowed to have a non-zero size. I know nothing more about the test, so I can't give any other effective comments at this time.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Josh Matthews [:jdm] from comment #2)
> With per-window private browsing enabled, the disk cache is allowed to have
> a non-zero size. I know nothing more about the test, so I can't give any
> other effective comments at this time.

Josh, we were working together on this test on bug 789987. So we would love to get feedback about the right behavior of memory and disk caches.

Comment 4

[Josh Matthews [:jdm]]

I think you'll need to be more specific about what feedback you're looking for. As I recall, we broke down bug 789987 into pretty well-specified steps in regards to the expected behaviour of the caches.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Josh Matthews [:jdm] from comment #4)
> I think you'll need to be more specific about what feedback you're looking
> for.

Hm, actually not sure what you are asking for here. What we have to know is the updated behavior of disk and memory caches regarding per the new window private-browsing mode - what this bug is about and you have commented earlier on.

Comment 6

[Josh Matthews [:jdm]]

The disk cache has no size constraints during or after testing; we only care that it doesn't not contain any entries that expose URLs visited in a private session. The memory cache should be 0 immediately after the last-pb-context-exited notification occurs.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Josh Matthews [:jdm] from comment #6)
> private session. The memory cache should be 0 immediately after the
> last-pb-context-exited notification occurs.

Which means that we should at best open two pb windows to test that. So we can check that items are still existent after the first pb window gets closed and all are removed when the second window is closed. If you agree with that we are good to go ahead. Thanks!

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

This will be fixed by Alex work on bug 818456.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Josh, can we hash out the right steps to do in the test now? It's not fully clear to me. Would we still need it in terms of any other added Mochitest for this new feature? If that's still the case can you please reorder the steps so it would make sense?

* Clear caches
* Check that caches don't have entries from the test pages
* Open Private Browsing Window
* Navigate to different pages
* Check that disk cache has no entries for the visited pages while the memory cache contains entries
* Open another Private Browsing window
* Close the first Private Browsing window
* Check that the memory cache still has entries for the visited pages
* Close the second Private Browsing window
* Check that the memory cache hasn't entries for the visited pages

Thanks

Comment 10

[Josh Matthews [:jdm]]

I don't really see the value of opening two private windows and checking that there are still cache entries after closing one of them.

* clear caches
* open private window
* navigate to test pages
* check disk cache has no entries for visited pages (there's no need to assert anything about the memory cache)
* close private window
* check that memory cache is empty

I don't see any advantage to making it more complicated than that.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Josh Matthews [:jdm] from comment #10)
> I don't really see the value of opening two private windows and checking
> that there are still cache entries after closing one of them.

So i probablly misunderstood the following comment you gave in comment 6:

The memory cache should be 0 immediately after the last-pb-context-exited notification occurs.


Thanks for clarifying.

Comment 12

[Andreea Matei [:AndreeaMatei]]

Josh, the error in the summary appears before opening a new PB window, we clear all cache in setupModule() through evictEntries() and assert that disk entries and disk size is 0.
But it appears disk size doesn't get cleared. Is this an expected behavior or just in PB and after closing the PB window there are no restrains to disk size?

Comment 13

[Josh Matthews [:jdm]]

Hmm, I'm surprised by that. You could try waiting for the cacheservice:empty-cache notification before continuing and see if that makes a difference.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Josh, I found bug 808997 has caused this, with the builds until 15th this test worked, started to fail on 16th when that patch was added.
I will check that notification as well.

Comment 15

[Josh Matthews [:jdm]]

That being said, I'm actually inclined not to bother which checking the reported totalSize of the cache. The fact that it reports zero entries is what matters, and that seems to always pass.

Comment 16

[Andreea Matei [:AndreeaMatei]]

> * check disk cache has no entries for visited pages (there's no need to
> assert anything about the memory cache)
> * close private window
> * check that memory cache is empty

Josh, shouldn't we check for the disk entries not containing the visited pages in PB again after exiting PB? Besides checking for the memory cache to be empty.

Comment 17

[Josh Matthews [:jdm]]

Sure, that ordering probably makes more sense.

Comment 18

[Andreea Matei [:AndreeaMatei]]

This stopped failing 2 weeks ago, along with the other dependencies of bug 822188.

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Because this test has been skipped for now as per bug 818456.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Lets continue on this bug so we can re-enable this test again.

Comment 21

[Andreea Matei [:AndreeaMatei]]

It's not reproducing anymore, the test is enabled now. Since Josh said in comment 2 and 15 that disk size cache has no constrains anymore, this was fixed in bug 818456.

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Thanks. Updating status of this bug to reflect the reality.