789987 - Create Mozmill test for Private Browsing cache handling

Status: RESOLVED FIXED

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

Description

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

On bug 787743 we fixed the problem where downloaded content during private browsing mode was still in cache after pb mode has been left. The following steps will cover what we have to do to ensure it will not break again in the future.

1. Open about:cache
2. Clear cache (use API instead of Clear Recent History dialog
3. Check that disk cache reports 0KB and have no entries (use API if possible)
4. Start Private Browsing
5. Navigate to different pages (different domains if necessary)
6. Check Disk Cache - there should be a couple cached items, particularly from the browsed websites
7. Stop private browsing mode
8. Check that disk cache reports 0KB and have no entries

Comment 1

[Josh Matthews [:jdm]]

Step 6 is incorrect; we want no disk cache entries while in PB mode. We should check that the memory cache has no entries after leaving PB mode, and it doesn't hut to check that the same is still true for the disk cache as well.

Comment 2

[(no longer active)]

Well, step 6 is incorrect in that the cache should not contain any entries at all from the websites that you browsed to.  With the fix that I'm working on in bug 787743 and the dependent bugs, it's possible for stuff that are not related to the user's browsing to end up in the disk cache (such as update pings, blocklist info pings, etc.)

Josh is right on about the memory cache though.

Comment 3

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

So that means:

[..]
6. Check that disk cache reports 0KB and has no entries, while the memory cache contains entries
7. Stop private browsing mode
8. Check that the memory cache reports 0K and has no entries

Comment 4

[(no longer active)]

(In reply to comment #3)
> So that means:
> 
> [..]
> 6. Check that disk cache reports 0KB and has no entries, while the memory cache
> contains entries

No:

6. Check the disk cache to make sure it doesn't have any entries to any of the websites that were browsed in private browsing mode.

Comment 5

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

But what about the memory cache? Is this also empty at this state, means before leaving the PB mode? You both haven't mentioned anything for it so far.

Comment 6

[Josh Matthews [:jdm]]

The memory cache can contain anything while in PB mode, and should contain nothing after exiting it.

Comment 7

[(no longer active)]

(In reply to comment #5)
> But what about the memory cache? Is this also empty at this state, means before
> leaving the PB mode? You both haven't mentioned anything for it so far.

No, the memory cache will get filled as you browse inside the private browsing mode, and it will get emptied right after you switch out of it.

Comment 8

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

So I'm confused now because that's exactly what I have given as step 6 in my comment 3. So why you mentioned it is wrong, Ehsan?

Comment 9

[(no longer active)]

(In reply to comment #8)
> So I'm confused now because that's exactly what I have given as step 6 in my
> comment 3. So why you mentioned it is wrong, Ehsan?

Because the disk cache might include entries which are not related to the user's browsing session, such as an update ping, for example.

Comment 10

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

Ah ok, so we should check for entries of the visited pages only:

1. Open about:cache
2. Clear cache (use API instead of Clear Recent History dialog
3. Check that disk cache reports 0KB and have no entries (use API if possible)
4. Start Private Browsing
5. Navigate to different pages (different domains if necessary)
6. Check that disk cache has entries for the visited pages (needs fix on bug 787743 first), while the memory cache contains entries
7. Stop private browsing mode
8. Check that caches have no entries

Comment 11

[Josh Matthews [:jdm]]

6 should read "6. Check that disk cache has NO entries for the visited pages (needs fix on bug 787743 first), while the memory cache contains entries", and 8 should specify that the caches have no entries for the visited pages.

Comment 12

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

Ok, looks like we have everything now. We will put this as a work item for next week.

Comment 13

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v0.1

I've created the test, but while testing I found a Nightlty bug: the memory cache is not empty after exiting the private browsing. I can reproduce this manually on all platforms.

On release though, it works perfectly.

I've emailed Anthony Hughes to see if this will get fixed in bug 787743 or if I should file another one.

Meanwhile, I'll appreciate your feedback regarding the test, so it will be ready on time, after the bug gets fixed.

Comment 14

[Josh Matthews [:jdm]]

Due to various design constraints, there's no guarantee that the memory cache will clear immediately after leaving PB mode. What happens if you add an observer for the "last-pb-context-exited" notification and then run an event in the future to check the cache size?

Comment 15

[u279076]

Andreea, I am not reproducing what you are seeing on Firefox 18.0a2 nor 19.0a1. I'm guessing you are hitting a condition as Josh points out. I would not expect your test to fail on all testruns based on comment 14 but the intermittent nature of clearing cache might make this hard to automate.

Does trying what Josh has suggested help at all?

Comment 16

[Andreea Matei [:AndreeaMatei]]

Anthony, what I've tried manually was clearing everything (about:cache will show 0 entries on disk and memory), enter private browsing, navigate (have about:cache show memory entries), exit private browsing and still had entries on about:cache.
It is possible that if we check after 30 seconds or so the data is correct. Will have to see how can we work around that.

Josh, tried a sleep of 3000ms after exiting private browsing and seems to do the trick, so you're right, it's not clearing immediately. Will check tomorrow the event idea. 

Thanks!

Comment 17

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

Comment on attachment 671851 [details] [diff] [review]
patch v0.1

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

Good start on this test Andreea! Lets figure out the remaining issues so we can get this landed soon.

::: tests/functional/testPrivateBrowsing/manifest.ini
@@ +5,5 @@
>  [testDisabledPermissions.js]
>  [testDownloadManagerClosed.js]
>  [testGeolocation.js]
>  [testKeyboardShortcut.js]
> +[testPivateBrowsingCache.js]

We are already under private browsing. So I don't see a need to include it in the filename.

::: tests/functional/testPrivateBrowsing/testPrivateBrowsingCache.js
@@ +19,5 @@
> +  controller = mozmill.getBrowserController();
> +
> +  // Create Private Browsing instance and set handler
> +  pb = new privateBrowsing.privateBrowsing(controller);
> +  pb.handler = pbStartHandler;

You can directly turn off the message. It's not necessary to handle that yourself. See the other private browsing tests.

@@ +35,5 @@
> +function testPrivateBrowsingCache() {
> +  // Clear cache
> +  cs = Cc["@mozilla.org/network/cache-service;1"]
> +          .getService(Ci.nsICacheService);
> +  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);

Does this run synchronous?

@@ +46,5 @@
> +  nodeCollector.root = nodeCollector.nodes[0];
> +
> +  var diskEntries = nodeCollector.queryNodes("td");
> +  expect.equal(diskEntries.nodes[0].textContent, "0", "Disk cache has no entries");
> +  expect.equal(diskEntries.nodes[2].textContent, "0 KiB", "There is no storage in use");

Why do we scrape about:cache and do not retrieve the current values from the interface directly?

@@ +73,5 @@
> +               "Memory cache has no entries");
> +
> +  var diskEntriesAfterPB = new elementslib.Selector(controller.tabs.activeTab, "#disk td");
> +  expect.equal(diskEntriesAfterPB.getNode().textContent, "0",
> +               "Disk cache has no entries");

Both checks look wrong to me given comment 11.

Comment 18

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v1

Updated considering the feedback.
I've replaced nodeCollector with visitEntries from: 
http://mxr.mozilla.org/mozilla-central/source/netwerk/cache/nsICacheService.idl#46
and constructed visitor in order to check both disk and memory devices.

In the end we need to wait about 2 seconds for the memory cache to update, as Josh explained. 
There are no entries listed in order to compare those with the pages visited in private browsing.

Comment 19

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

Comment on attachment 673249 [details] [diff] [review]
patch v1

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

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// Include the required modules
> +var {expect} = require("../../../lib/assertions");
> +var domUtils = require("../../../lib/dom-utils");

domUtils is not used so please remove.

@@ +7,5 @@
> +var domUtils = require("../../../lib/dom-utils");
> +var privateBrowsing = require("../../../lib/private-browsing");
> +var tabs = require("../../../lib/tabs");
> +
> +const CACHE_PAGE_URL = "about:cache";

Please move it below the LOCAL_TEST_PAGES constants and call it simply CACHE_PAGE

@@ +11,5 @@
> +const CACHE_PAGE_URL = "about:cache";
> +
> +const LOCAL_TEST_FOLDER = collector.addHttpResource('../../../data/');
> +const LOCAL_TEST_PAGES = [LOCAL_TEST_FOLDER + 'layout/mozilla_grants.html',
> +                          LOCAL_TEST_FOLDER + 'layout/mozilla_mission.html'];

I highly suggest we are making use of mozqa.com and using different subdomains. Its one of the main reasons why we have to write the test because with Mochitest we can't hit the external network.

@@ +30,5 @@
> + */
> +function testPrivateBrowsingCache() {
> +  // Get entries count and actual disk size
> +  var visitor = {
> +    visitDevice: function(deviceID, deviceInfo) {

nit: blank after function.

@@ +36,5 @@
> +        diskEntries = deviceInfo.entryCount;
> +        diskSize = deviceInfo.totalSize;
> +      } else if (deviceID == "memory") {
> +        memoryEntries = deviceInfo.entryCount;
> +      }

The variables you assign values to are not defined. You have to do that before.

@@ +52,5 @@
> +  pb.showPrompt = false;
> +
> +  // Clear cache
> +  cs = Cc["@mozilla.org/network/cache-service;1"]
> +          .getService(Ci.nsICacheService);

nit: please follow our coding style. The dot is at the end of the first line. Also fix the indentation.

@@ +60,5 @@
> +  controller.waitForPageLoad();
> +
> +  cs.visitEntries(visitor);
> +  expect.equal(diskEntries, 0, "Disk cache has no entries");
> +  expect.equal(diskSize, 0, "There is no disk storage in use");

I would also add a check for the memory cache entries which should also be empty, right?

@@ +74,5 @@
> +  controller.waitForPageLoad();
> +
> +  var memoryEntriesInPB = new elementslib.Selector(controller.tabs.activeTab, "#memory td");
> +  expect.notEqual(memoryEntriesInPB.getNode().textContent, "0",
> +                  "Memory cache contains entries in PB");

You should do that via visitEntries() again and scrape the data via the ui. Also I miss the check for the disk cache here.

@@ +78,5 @@
> +                  "Memory cache contains entries in PB");
> +
> +  pb.stop();
> +
> +  expect.waitFor(function () {

I don't think waitFor() is the right solution here in combination with number of cache entries. I would love to see that there is an event we can listen for. Hopefully Ehsan or Josh could point us to that.

@@ +83,5 @@
> +    controller.open(CACHE_PAGE_URL);
> +    controller.waitForPageLoad();
> +    cs.visitEntries(visitor);
> +
> +    return (memoryEntries === 0 && diskEntries === 0);

That's wrong. As you can see for step 8 there can be entries in the cache. What we have to check is that none of those are the URLs we have visited.

Comment 20

[Josh Matthews [:jdm]]

As I mentioned earlier, you can wait for the observer notification (ie. nsIObserverService, nsIObserver) "last-pb-context-exited".

Comment 21

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

Sorry, missed that part. So yes, please use that observer notification together with waitFor().

Comment 22

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v2

Updated as requested. 
For checking the visited pages into the list entries, we use entryInfo.key in visitEntry. We take each entry and create an array of the list, so we can compare later.

We can't check the memory before private browsing because there is none (I get undefined). As you can see, at that stage we only have disk cache and offline cache in the about:cache. 

Added the observer and an if in case we do have cache entries updated after exiting PB, to check for the visited pages in those.

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 676646 [details] [diff] [review]
patch v2

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

This is failing for me on latest Nightly and Aurora builds. The following is the console output:

$ mozmill -t tests/functional/testPrivateBrowsing/testAboutCache.js -b /Applications/FirefoxNightly.app/ --show-errors
2012-11-02 12:45:12.659 firefox-bin[8872:707] invalid drawable
TEST-START | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | setupModule
TEST-PASS | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | testAboutCache.js::setupModule
TEST-START | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | testPrivateBrowsingCache
ERROR | Test Failure: {"fail": {"stack": {"lineNumber": 186, "caller": {"lineNumber": 431, "caller": {"lineNumber": 118, "caller": {"lineNumber": 595, "caller": {"lineNumber": 665, "caller": {"lineNumber": 711, "caller": {"lineNumber": 544, "caller": {"lineNumber": 723, "caller": {"lineNumber": 179, "caller": {"lineNumber": 183, "caller": {"lineNumber": 283, "caller": {"lineNumber": 0, "caller": null, "name": null, "filename": null}, "name": null, "filename": "resource://jsbridge/modules/server.js"}, "name": null, "filename": "resource://jsbridge/modules/server.js"}, "name": null, "filename": "resource://jsbridge/modules/server.js"}, "name": null, "filename": "resource://mozmill/modules/frame.js"}, "name": null, "filename": "resource://mozmill/modules/frame.js"}, "name": null, "filename": "resource://mozmill/modules/frame.js"}, "name": null, "filename": "resource://mozmill/modules/frame.js"}, "name": null, "filename": "resource://mozmill/modules/frame.js"}, "name": "testPrivateBrowsingCache", "filename": "resource://mozmill/modules/frame.js -> file:///Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js"}, "name": "Expect_waitFor", "filename": "resource://mozmill/stdlib/securable-module.js -> file:///Users/dhunt/workspace/mozmill-tests/default/lib/assertions.js"}, "name": "Expect__test", "filename": "resource://mozmill/stdlib/securable-module.js -> file:///Users/dhunt/workspace/mozmill-tests/default/lib/assertions.js"}, "message": "Private browsing was exited", "lineNumber": 118, "name": "testPrivateBrowsingCache", "fileName": "file:///Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js"}}
TEST-UNEXPECTED-FAIL | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | testAboutCache.js::testPrivateBrowsingCache
TEST-START | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | teardownModule
TEST-PASS | /Users/dhunt/workspace/mozmill-tests/default/tests/functional/testPrivateBrowsing/testAboutCache.js | testAboutCache.js::teardownModule
INFO Passed: 2
INFO Failed: 1
INFO Skipped: 0

It passes on latest Beta and Release builds.

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +9,5 @@
> +
> +const TEST_DOMAINS = ["http://domain1.mozqa.com",
> +                      "http://domain2.mozqa.com"];
> +
> +const CACHE_PAGE = "about:cache";

This is no longer needed.

@@ +10,5 @@
> +const TEST_DOMAINS = ["http://domain1.mozqa.com",
> +                      "http://domain2.mozqa.com"];
> +
> +const CACHE_PAGE = "about:cache";
> +const LAST_PB_CONTEXT_EXITED = "last-pb-context-exited";

I'm not convinced this needs to be a constant.

@@ +65,5 @@
> +  cs = Cc["@mozilla.org/network/cache-service;1"].
> +       getService(Ci.nsICacheService);
> +  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);
> +
> +  controller.open(CACHE_PAGE);

You're not scraping the about:cache page any more, so you don't need this.

@@ +79,5 @@
> +    controller.open(aPage);
> +    controller.waitForPageLoad();
> +  });
> +
> +  controller.open(CACHE_PAGE);

As above, there is no longer a need for this.

@@ +98,5 @@
> +  var observerService = Cc["@mozilla.org/observer-service;1"].
> +                        getService(Ci.nsIObserverService);
> +
> +  // Create flag visible to both the eval and the observer object
> +  var finishedFlag = {

Can this simply be a boolean?

@@ +106,5 @@
> +  // Set up an observer so we get notified when we exited PB
> +  let observer = {
> +    observe: function (aSubject, aTopic, aData) {
> +      observerService.removeObserver(this, LAST_PB_CONTEXT_EXITED);
> +      finishedFlag.state = true;

I believe we should be asserting that there are 0 memory entries here, and that any disk entries are not related to the pages visited in private browsing.

@@ +116,5 @@
> +    cs.visitEntries(visitor);
> +    return finishedFlag.state;
> +  }, "Private browsing was exited");
> +
> +  controller.open(CACHE_PAGE);

As above, there is no longer a need for this.

Comment 24

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Also, could you link to a report running against Firefox 15.0 showing that this test would pick up the original regression.

Comment 25

[Josh Matthews [:jdm]]

The test fails on nightly because last-pb-context-exited now can fire synchronously when leaving the global private browsing mode (but is not guaranteed to). that means the observer should be in place before calling pb.stop().

Comment 26

[Andreea Matei [:AndreeaMatei]]

Yes Josh, we figured it out on IRC, while running the test. It's all done now, here are the reports:
For Nightly passes:
* http://mozmill-crowd.blargon7.com/#/functional/report/190d57cf64e3c5c3996c0c105967367c

For 15.0:
* http://mozmill-crowd.blargon7.com/#/functional/report/190d57cf64e3c5c3996c0c105967bcf2

* http://mozmill-crowd.blargon7.com/#/functional/report/190d57cf64e3c5c3996c0c105966a5e5

The patch is in internal review at this moment, updated after Dave's review.

I wonder, we should expect the memory to be 0 and to assert the disk cache list doesn't contain the visited pages or assert both of them?

Comment 27

[Josh Matthews [:jdm]]

Both conditions should be asserted as a condition for the test to pass.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3

Removed the CACHE_PAGE since we no longer need to open it, the constant "last-pb-context-exited" and added the observer in pb mode. 
After exiting PB mode we assert that we have no memory entries and the pages visited in PB are not present in disk entries list.

Comment 29

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 678265 [details] [diff] [review]
patch v3

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

A couple of minor issues, however I would also like a final review from Henrik.

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +97,5 @@
> +      finishedFlag = true;
> +    }
> +  }
> +  observerService.addObserver(observer, "last-pb-context-exited", false);
> +  

Nit: trailing whitespace

@@ +108,5 @@
> +  cs.visitEntries(visitor);
> +
> +  assert.equal(memoryEntriesCount, 0, "Memory cache has no entries after PB mode");
> +
> +  if (diskEntriesCount !== 0) {

We shouldn't need this if statement.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v3.1

Removed if statement and whitespace.

Comment 31

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

Comment on attachment 680030 [details] [diff] [review]
patch v3.1

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

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +27,5 @@
> +function testPrivateBrowsingCache() {
> +  // Get entries count and actual disk size
> +  var diskEntriesCount, memoryEntriesCount, diskSize, diskEntry, memoryEntry,
> +      diskEntries = [], memoryEntries = [];
> +  var visitor = {

Please fix the comment. It applies to the cs.visitEntries() call but not for the declaration here. Also please add a blank line between both declarations.

@@ +28,5 @@
> +  // Get entries count and actual disk size
> +  var diskEntriesCount, memoryEntriesCount, diskSize, diskEntry, memoryEntry,
> +      diskEntries = [], memoryEntries = [];
> +  var visitor = {
> +    visitDevice: function (deviceID, deviceInfo) {

nit: We have the coding style to add the 'a' prefix to parameters.

@@ +36,5 @@
> +      }
> +      else if (deviceID == "memory") {
> +        memoryEntriesCount = deviceInfo.entryCount;
> +      }
> +      return deviceID;

Why do we have to return the deviceID? You should return true instead.

@@ +48,5 @@
> +      }
> +      else if (deviceID == "memory") {
> +        memoryEntry = entryInfo.key;
> +        memoryEntries.push(memoryEntry);
> +        return memoryEntries;

Same as above. Please return true in both cases. Also use triple-operators for the comparisons.

@@ +55,5 @@
> +  }
> +
> +  // Make sure we are not in PB mode and don't show a prompt
> +  pb.enabled = false;
> +  pb.showPrompt = false;

This belongs to the setupModule() method.

@@ +60,5 @@
> +
> +  // Clear cache
> +  cs = Cc["@mozilla.org/network/cache-service;1"].
> +       getService(Ci.nsICacheService);
> +  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);

Same for those 3 lines.

@@ +64,5 @@
> +  cs.evictEntries(Ci.nsICache.STORE_ANYWHERE);
> +
> +  cs.visitEntries(visitor);
> +  expect.equal(diskEntriesCount, 0, "Disk cache has no entries");
> +  expect.equal(diskSize, 0, "There is no disk storage in use");

Shouldn't that be assert calls?

@@ +78,5 @@
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    diskEntries.forEach(function (aEntry) {
> +      expect.notContain(aEntry, aPage,
> +                        "Pages visited are not present in PB disk cache entries");
> +    });

Why do we have to iterate over diskEntries and can't simply use its indexOf() method? Also please fix the message so it refers to the current page and not all pages.

@@ +84,5 @@
> +
> +  expect.notEqual(memoryEntriesCount, 0, "Memory cache contains entries in PB");
> +
> +  var observerService = Cc["@mozilla.org/observer-service;1"].
> +                        getService(Ci.nsIObserverService);

nit: please call it 'obs' so we are in sync with Services.jsm.

@@ +87,5 @@
> +  var observerService = Cc["@mozilla.org/observer-service;1"].
> +                        getService(Ci.nsIObserverService);
> +
> +  // Create flag visible to both the eval and the observer object
> +  var finishedFlag = false;

Looks like the comment needs an update.

@@ +98,5 @@
> +    }
> +  }
> +  observerService.addObserver(observer, "last-pb-context-exited", false);
> +
> +  pb.stop();

We should include the observer code in the private browsing class, so stop() takes care of it. Feel free to work on that in a separate bug.

@@ +112,5 @@
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    diskEntries.forEach(function (aEntry) {
> +      assert.notContain(aEntry, aPage,
> +                        "Pages visited in PB are not present in disk cache entries");
> +    });

Same as above for the loop and indexOf().

Comment 32

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4

Got unblocked from bug 812114 so I've addressed all requests. 

These are the reports, passing for default:
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d371672507a9

Failing on Firefox 15:
http://mozmill-crowd.blargon7.com/#/functional/report/352218d7e3125c857fc1d37167266041

Comment 33

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 686617 [details] [diff] [review]
patch v4

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

::: tests/functional/testPrivateBrowsing/testAboutCache.js
@@ +33,5 @@
> +/**
> + * Test to check caching in Private Browsing
> + */
> +function testPrivateBrowsingCache() {
> +  // Get entries information for both disk and memory devices

> Please fix the comment. It applies to the cs.visitEntries() call but not for
> the declaration here.

This comment still needs to be moved to the cs.visitEntries() call rather than at the declaration.

@@ +75,5 @@
> +  });
> +
> +  cs.visitEntries(visitor);
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    expect.ok(diskEntries.toString().indexOf(aPage) === -1,

Why use toString() here?

@@ +76,5 @@
> +
> +  cs.visitEntries(visitor);
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    expect.ok(diskEntries.toString().indexOf(aPage) === -1,
> +              "Current visited page is not present in PB disk cache entries");

Include the current page in the message

@@ +87,5 @@
> +  cs.visitEntries(visitor);
> +  assert.equal(memoryEntriesCount, 0, "Memory cache has no entries after PB mode");
> +
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    assert.ok(diskEntries.toString().indexOf(aPage) === -1,

As above, no need for toString()

@@ +88,5 @@
> +  assert.equal(memoryEntriesCount, 0, "Memory cache has no entries after PB mode");
> +
> +  TEST_DOMAINS.forEach(function (aPage) {
> +    assert.ok(diskEntries.toString().indexOf(aPage) === -1,
> +              "Pages visited in PB are not present in disk cache entries");

Please use the current page in the message

Comment 34

[Andreea Matei [:AndreeaMatei]]

Attachment: patch v4.1

Moved the comment, added the page in the messages and removed toString().

Comment 35

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 687060 [details] [diff] [review]
patch v4.1

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

Looks great! Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/09c6d58a2763

Comment 36

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

Not fixed yet. This is a test for a chemspill release of Firefox. So we have to get this backported to all branches down to 17. Please check results in the next days and if it works correctly on those other target branches.

Comment 37

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

Landed on all other branches:

http://hg.mozilla.org/qa/mozmill-tests/rev/7c84344218cd (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/0de54e7b65e7 (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/944306ff602e (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/b64327ba1953 (esr17)

Comment 38

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

Backed out the test from esr17 due to test failures. See bug 818926. This will be fixed by bug 812114. Reopening bug given that it's not fully fixed yet.

Comment 39

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

Backout is:
http://hg.mozilla.org/qa/mozmill-tests/rev/d8e3158e2a6b (esr17)

Comment 40

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

I think we should update the test so we connect via HTTPS to mozqa.com in private browsing mode. That way we could cover a possible issue as given in bug 787743 comment 86.

Comment 41

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

(In reply to Henrik Skupin (:whimboo) from comment #40)
> I think we should update the test so we connect via HTTPS to mozqa.com in
> private browsing mode. That way we could cover a possible issue as given in
> bug 787743 comment 86.

Well, lets wait until a new bug has been opened for it - if it's a valid issue.

Andreea, do we have any progress for the ESR17 case?

Comment 42

[Andreea Matei [:AndreeaMatei]]

Yes, I've looked again on Friday and with many dumps for all blocks, the cause is the observer is not getting registered, as Josh explained.

The dump in the observer is not executed, so the flag doesn't get changed there.
In ESR17 we don't have multiple windows, it's the old switch to PB mode. 
I looked to see another event worth waiting for or maybe somehow to force PB mode to close in a catch block added to our try-finally.

Comment 43

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

Hm, where is the comment from Josh? I cannot find it on this bug. So we most likely register our observer correctly but the notification is most likely not fired. Which could mean we are still in PB mode. Can you reproduce that locally?

Comment 44

[Andreea Matei [:AndreeaMatei]]

It was on bug 812114 at comment 25.
I reproduced only on testruns (just with the private browsing folder), but rarely, 1 in 15-20 runs.
These are my reports from Friday, tried with 2-3 tests:

http://mozmill-crowd.blargon7.com/#/functional/reports?branch=17.0&platform=Mac&from=2013-01-18&to=2013-01-18

Comment 45

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

Relanded patch for esr17:
http://hg.mozilla.org/qa/mozmill-tests/rev/a19db99a2fb7

Lets cross fingers that it will work this time. Looks like we are done here.