827752 - Create Mozmill test to verify that Flash content respects Firefox's private browsing

Status: RESOLVED FIXED

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

Description

[Alex Lakatos[:AlexLakatos]]

Here are the tweaked steps based on the ones proposed by [:mwobensmith]

1. Open this URL in a private broswing window:
http://people.mozilla.org/~mwobensmith/private_browsing/set_flash_cookie.html
2. Enter a unique value in the text field
3. Click the link to set the value.
4. Verify an alert box appears indicating success.
5. Open another window, but NOT private. Perform the following steps in the new window.
6. Open this URL:
http://people.mozilla.org/~mwobensmith/private_browsing/get_flash_cookie.html
7. Click on the link to retrieve the Flash cookie.
8. View the new text written to the page.

I would propose all the external url's referenced be created as test files in the data/private_browsing/ directory.

Comment 1

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

This test looks pretty easy to implement so I will add it for this weeks sprint.

Comment 2

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

Matt, can you please add the implementation bug to the blocking bugs list? Thanks.

Comment 3

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

Please do not forget to set the status of the bug to assigned when you want to work on a bug. Thanks.

Do we have an update here?

Comment 4

[Alex Lakatos[:AlexLakatos]]

Matt is still working on the test files. The initial ones only worked for Mac. Last time I talked with him he finished the ones for Windows and was working on Linux.

Comment 5

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

Keep in mind that only 4 days are left until this new test should be completed. So you should work closer with him and I would expect updates here how the process goes.

Comment 6

[Matt Wobensmith [:mwobensmith][:matt:]]

The above tests should now work cross-platform. Alex - have a look.

Henrik - should I assign the bug to Alex directly? And can you explain what you'd like me to do w/r/t comment 2? Thanks.

Comment 7

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

Matt, simply create a bug in Mozilla QA / Infrastructure, so that we can get those tests landed in the litmus-data (http://hg.mozilla.org/qa/litmus-data/) repository. Mark this bug as blocking that one.

Comment 8

[Matt Wobensmith [:mwobensmith][:matt:]]

Thanks Henrik - done.

Comment 9

[Daniela Petrovici]

(In reply to Matt Wobensmith from comment #6)
> The above tests should now work cross-platform. Alex - have a look.
> 
I verified the test manually on Linux x86 and it doesn't work. Steps I have performed:
1) Open: http://people.mozilla.org/~mwobensmith/private_browsing/set_flash_cookie.html
2) Enter a unique value in the text field
3) Click the link to set the value.

RESULT: Nothing happens.
NOTE: On MAC OS and Windows, the alert box appears in this step with "Success!" message

4) Open this URL:
http://people.mozilla.org/~mwobensmith/private_browsing/get_flash_cookie.html
5) Click the link to retrieve the cookie.

RESULT: Nothing happens.
Note: On MAC OS and Windows, the text undefined appears.

Matt, can you please fix the issue with the page on Linux?

Comment 10

[Daniela Petrovici]

Please disregard the previous comment. The issue was that the plugin was not install on my Linux box

Comment 11

[Matt Wobensmith [:mwobensmith][:matt:]]

New steps - to be clearer:

1. Open this URL in a private browsing window:
http://people.mozilla.org/~mwobensmith/private_browsing/set_flash_cookie.html
2. Enter a unique value in the text field
3. Click the link to set the value.
4. Verify that text appears indicating success.
5. Open another window, but NOT private. Perform the following steps in the new window.
6. Open this URL:
http://people.mozilla.org/~mwobensmith/private_browsing/get_flash_cookie.html
7. Click on the link to retrieve the Flash cookie.
8. View the text written to the page.
9. Verify that the text from the previous step *does not* match  the text from step 2.

Comment 12

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

So I have a dumb question, how can we remove the flash cookie? We would need such a possibility to run this test.

Comment 13

[Matt Wobensmith [:mwobensmith][:matt:]]

I can add something to the test so that it resets the Flash cookie every time it opens. Would that suffice?

Comment 14

[Matt Wobensmith [:mwobensmith][:matt:]]

Attachment: HTML/SWF/AS/FLA files for Flash/private browsing test

Added support to remove any previous Flash cookie when test starts.

Comment 15

[Matt Wobensmith [:mwobensmith][:matt:]]

Apologies, I uploaded the patch to the wrong bug. Ignore the one here - I'll add it to related bug 831406 shortly.

Comment 16

[Daniela Petrovici]

Attachment: patch v1.0

This patch isn't final since the files are not added yet on mozqa.com and we don't have a way to delete the cookie at the moment, but I wanted to get a feedback on the intermediary work I did.

Comment 17

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

Comment on attachment 708490 [details] [diff] [review]
patch v1.0

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

We shouldn't continue on this patch before the testcase made it into the testcase-data repository. There will be a couple of changes which are necessary to make it better usable with automation. For details see my inline comments. Also there are a lot of white-space issues, which you should try to avoid in general.

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +30,5 @@
> +  pbWindow.controller.open(SET_COOKIE_TEST_PAGE);
> +  pbWindow.controller.waitForPageLoad();
> +  
> +  // Enter a unique value for the cookie
> +  var unique = getUniqueId();

I don't think there is need to calculate the id from fresh each time. Given that we wipe it out after the test it will always be unique. We might want to reset the cookie in setup for safety.

@@ +33,5 @@
> +  // Enter a unique value for the cookie
> +  var unique = getUniqueId();
> +  var setCookie = new elementslib.Name(pbWindow.controller.tabs.activeTab, "myValue");
> +
> +  assert.ok(setCookie.exists(), "Input field has been found");

Why do we need this assert call?

@@ +38,5 @@
> +
> +  pbWindow.controller.type(setCookie, unique);
> +
> +  var linkToSetCookie = new elementslib.Link(pbWindow.controller.tabs.activeTab, 
> +                                             "Click here to create a Flash cookie.");

Any element we have to work on will get a unique id. Never operate via Link. So the testcase needs to be updated.

@@ +51,5 @@
> +  controller.open(GET_COOKIE_TEST_PAGE);
> +  controller.waitForPageLoad();
> +
> +  var linkToGetCookie = new elementslib.Link(controller.tabs.activeTab, 
> +                                             "Click here to retrieve Flash cookie.");

Same here with the Link element.

@@ +56,5 @@
> +  controller.click(linkToGetCookie);
> +
> +  var flashCookieElement = new elementslib.ID(controller.tabs.activeTab, "myDiv");
> +  var flashCookieText = flashCookieElement.getNode().textContent;
> +  assert.ok(flashCookieText.indexOf(unique) === -1, "The cookie is visible"); 

I believe you want to do an assert.contains() here.

Comment 18

[Daniela Petrovici]

Attachment: patch v1.1

Modified the patch to use the files added in bug 831406

Comment 19

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

Comment on attachment 719443 [details] [diff] [review]
patch v1.1

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

Looks fine to me. Only some things to update.

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +7,5 @@
> +var privateBrowsing = require("../../../lib/ui/private-browsing");
> +var tabs = require("../../../lib/tabs");
> +
> +const COOKIE_TEST_FOLDER = "http://mozqa.com/data/firefox/plugins/flash/cookies/"
> +const COOKIE_TEST_PAGE = COOKIE_TEST_FOLDER + "flash_cookie.html";

Please remove the COOKIE_ prefix and stick to the naming schema we have in other tests.

@@ +16,5 @@
> +  tabs.closeAllTabs(aModule.controller);
> +}
> +
> +function teardownModule(aModule) {
> +  aModule.pbWindow.close();

If we can't load the page in the normal window, we will not be able to clear the value. So I would suggest we are issuing an API call to clear all cookies, which also has to delete Flash cookies. Please verify that.

@@ +27,5 @@
> +  pbWindow.controller.open(COOKIE_TEST_PAGE);
> +  pbWindow.controller.waitForPageLoad();
> +
> +  // Enter a unique value for the cookie
> +  var cookieValue = "cookieValue";

You are using it a couple of times in this test, so it's a constant you can put at the top of the page.

@@ +37,5 @@
> +  controller.click(setCookie);
> +
> +  // Verify the cookie value was set properly
> +  var resultFieldPB = new elementslib.ID(pbWindow.controller.tabs.activeTab, "result_get");
> +  var resultFieldPBValue = resultFieldPB.getNode().value;

No need for this extra temporary variable.

@@ +41,5 @@
> +  var resultFieldPBValue = resultFieldPB.getNode().value;
> +  assert.equal(resultFieldPBValue, cookieValue,
> +               "Cookie value is displayed in private mode");
> +
> +  controller.open(COOKIE_TEST_PAGE);

Please add a comment that this is now not the pb window. It's easy to miss.

@@ +50,5 @@
> +  controller.click(getCookie);
> +
> +  // Verify the cookie value is undefined
> +  var resultField = new elementslib.ID(controller.tabs.activeTab, "result_get");
> +  var resultFieldValue = resultField.getNode().value;

Same as above.

@@ +54,5 @@
> +  var resultFieldValue = resultField.getNode().value;
> +  assert.notEqual(resultFieldValue, cookieValue,
> +                  "Cookie value is not displayed in normal mode");
> +
> +  //Reset the cookie value

nit: Missing blank.

Comment 20

[Daniela Petrovici]

Attachment: patch with dumps and removeAll() cookies

> > +function teardownModule(aModule) {
> > +  aModule.pbWindow.close();
> 
> If we can't load the page in the normal window, we will not be able to clear
> the value. So I would suggest we are issuing an API call to clear all
> cookies, which also has to delete Flash cookies. Please verify that.

The API call that I have used does not seem to work on the Flash cookies. It deletes the cookies that are created, but the Flash cookie remains present in /home/<user>/.macromedia.

I have attached the patch containing the dumps and the API call that I used.

Comment 21

[Daniela Petrovici]

Attachment: log file with dumps for cookies

Comment 22

[Daniela Petrovici]

Attachment: screenshot using BetterPrivacy add-on and Firefox preferences

I have also added a screenshot showing the flash cookie in BetterPrivacy add-on, that is not present in cookie manager in Firefox Preferences -> Privacy.

I could add another method for clearing the cookie that is run even if the test is failing - it would contain: opening the page and clearing the cookie - although if we were unable to load the page once, I am not sure if this would help by trying again.

Since removeAll() does not work, I am not sure how to delete the LSO in case we fail when loading the page in normal mode.

Comment 23

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

We can use the clear recent history window to get rid of those Flash cookies.

Comment 24

[Daniela Petrovici]

Attachment: patch v1.2

Modified patch based on feedback and tested that the flash cookie is deleted when using the Clear Recent History dialog

Comment 25

[Daniela Petrovici]

Comment on attachment 721264 [details] [diff] [review]
patch v1.2

Forgot to modify the manifest and add reports. I will update the patch.

Comment 26

[Daniela Petrovici]

Attachment: patch v1.3

Reports for Linux: http://mozmill-crowd.blargon7.com/#/functional/report/00ba6b3792451db672d192b3682b24e4
Report for Windows: http://mozmill-crowd.blargon7.com/#/functional/report/00ba6b3792451db672d192b3682be65c
Report for MAC: http://mozmill-crowd.blargon7.com/#/functional/report/00ba6b3792451db672d192b3682bfcb6

Comment 27

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

Comment on attachment 721624 [details] [diff] [review]
patch v1.3

Please stick to our agreement and ask review from Andreea only first.

Comment 28

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 721624 [details] [diff] [review]
patch v1.3

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

Just a few minor things remaining.

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +13,5 @@
> +
> +const TEST_FOLDER = "http://mozqa.com/data/firefox/plugins/flash/cookies/"
> +const TEST_PAGE = TEST_FOLDER + "flash_cookie.html";
> +
> +const TIMEOUT = 5000;

Where are we using this?

@@ +18,5 @@
> +
> +function setupModule(aModule) {
> +  aModule.controller = mozmill.getBrowserController();
> +  aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +  tabs.closeAllTabs(aModule.controller);

Please add a blank line above to separate declarations from method calls.

@@ +73,5 @@
> +
> +/**
> + * Accesses the clear recent history dialog and deletes the cookies
> + */
> +function clearCookiesHandler(controller) {

Please use aController as parameter.

Comment 29

[Daniela Petrovici]

Attachment: patch v1.4

Modified patch based on review.
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/172b2945b09ec259eb6e92f64415c69e
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/172b2945b09ec259eb6e92f6440e4477
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/172b2945b09ec259eb6e92f6441a6d28

Comment 30

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 722276 [details] [diff] [review]
patch v1.4

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

Looks good for me now!

Comment 31

[Daniela Petrovici]

Comment on attachment 722276 [details] [diff] [review]
patch v1.4

Also requesting review from Henrik, Dave since this is a new test

Comment 32

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

Comment on attachment 722276 [details] [diff] [review]
patch v1.4

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

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +33,5 @@
> +}
> +
> +function testCheckFlashCookie() {
> +  pbWindow.open(controller);
> +  pbWindow.controller.waitForPageLoad();

Why do we have to wait for the page being loaded? I don't think that's necessary.

@@ +38,5 @@
> +
> +  pbWindow.controller.open(TEST_PAGE);
> +  pbWindow.controller.waitForPageLoad();
> +
> +  // Enter a unique value for the cookie

nit: 'an'

@@ +49,5 @@
> +
> +  // Verify the cookie value was set properly
> +  var resultFieldPB = new elementslib.ID(pbWindow.controller.tabs.activeTab, "result_get");
> +  assert.equal(resultFieldPB.getNode().value, COOKIE_VALUE,
> +               "Cookie value is displayed in private mode");

I would also clear the cookie in setupModule() to be sure we don't have a false positive.

@@ +66,5 @@
> +                  "Cookie value is not displayed in normal mode");
> +
> +  // Reset the cookie value
> +  var clearCookie = new elementslib.ID(pbWindow.controller.tabs.activeTab, "clearCookie");
> +  pbWindow.controller.click(clearCookie);

Please add another check that the cookie has been unset. I feel that's kinda important for our test.

@@ +82,5 @@
> +
> +  var clearButton = new elementslib.Lookup(aController.window.document,
> +                                           '/id("SanitizeDialog")' +
> +                                           '/anon({"anonid":"dlg-buttons"})' +
> +                                           '/{"dlgtype":"accept"}');

I would love if we could clear the cookies directly via an API call. Handling the UI of the CRH dialog in tearDown is not the best solution. Sorry, that I wasn't that clear with my answer but I thought it's known that we don't want to use an UI path here.

Comment 33

[Daniela Petrovici]

Attachment: patch v1.5

(In reply to Henrik Skupin (:whimboo) from comment #32)
> Comment on attachment 722276 [details] [diff] [review]
> patch v1.4
> 
> Review of attachment 722276 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/functional/testPrivateBrowsing/testFlashCookie.js
> @@ +33,5 @@
> > +}
> > +
> > +function testCheckFlashCookie() {
> > +  pbWindow.open(controller);
> > +  pbWindow.controller.waitForPageLoad();
> 
> Why do we have to wait for the page being loaded? I don't think that's
> necessary.

The test fails intermittently on three different machines because we are not waiting for the pbWindow to be loaded before trying to open the TEST_PAGE. When the fail occurs pbWindow.controller.waitForPageLoad() waits for the private browsing window to be loaded, not the test page.

The test http://hg.mozilla.org/qa/mozmill-tests/file/abb82ac6a81d/tests/functional/testPrivateBrowsing/testAboutCache.js has the exact same problem.
We are not waiting for the Private browsing page to be loaded before trying to load the first test page (in this line http://hg.mozilla.org/qa/mozmill-tests/file/abb82ac6a81d/tests/functional/testPrivateBrowsing/testAboutCache.js#l67). After the private browsing window is loaded, the problem does not occur anymore. testAboutCache.js does not fail in this case because we are not checking which of the pages we have loaded, just that they were.

> @@ +38,5 @@
> > +
> > +  pbWindow.controller.open(TEST_PAGE);
> > +  pbWindow.controller.waitForPageLoad();
> > +
> > +  // Enter a unique value for the cookie
> 
> nit: 'an'
> 
> @@ +49,5 @@
> > +
> > +  // Verify the cookie value was set properly
> > +  var resultFieldPB = new elementslib.ID(pbWindow.controller.tabs.activeTab, "result_get");
> > +  assert.equal(resultFieldPB.getNode().value, COOKIE_VALUE,
> > +               "Cookie value is displayed in private mode");
> 
> I would also clear the cookie in setupModule() to be sure we don't have a
> false positive.
> 
> @@ +66,5 @@
> > +                  "Cookie value is not displayed in normal mode");
> > +
> > +  // Reset the cookie value
> > +  var clearCookie = new elementslib.ID(pbWindow.controller.tabs.activeTab, "clearCookie");
> > +  pbWindow.controller.click(clearCookie);
> 
> Please add another check that the cookie has been unset. I feel that's kinda
> important for our test.
> 
> @@ +82,5 @@
> > +
> > +  var clearButton = new elementslib.Lookup(aController.window.document,
> > +                                           '/id("SanitizeDialog")' +
> > +                                           '/anon({"anonid":"dlg-buttons"})' +
> > +                                           '/{"dlgtype":"accept"}');
> 
> I would love if we could clear the cookies directly via an API call.
> Handling the UI of the CRH dialog in tearDown is not the best solution.
> Sorry, that I wasn't that clear with my answer but I thought it's known that
> we don't want to use an UI path here.

I have created a function to clear history through the API as it is done here: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js#125 - this code is related to clearing LSO Flash cookies through Clear Recent History menu (based on bug 625496)

Comment 34

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

(In reply to Daniela Petrovici from comment #33)
> When the fail occurs pbWindow.controller.waitForPageLoad() waits for the
> private browsing window to be loaded, not the test page.

Why do you think that? waitForPageLoad() should tell you that this is not true. This method has nothing to do with the the private browsing window loaded. It has been written for content documents.

You should file a bug for that problem given that we have to fix it, and add a reference to this line of code. Mark this bug and the one for about:cache dependent on the newly filed one.

> I have created a function to clear history through the API as it is done
> here:
> http://mxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.
> js#125 - this code is related to clearing LSO Flash cookies through Clear
> Recent History menu (based on bug 625496)

Good. Just make sure to make it as easy as possible. We don't need a range. We want to always clear everything.

Comment 35

[Daniela Petrovici]

Attachment: log failed

(In reply to Henrik Skupin (:whimboo) from comment #34)
> (In reply to Daniela Petrovici from comment #33)
> > When the fail occurs pbWindow.controller.waitForPageLoad() waits for the
> > private browsing window to be loaded, not the test page.
> 
> Why do you think that? waitForPageLoad() should tell you that this is not
> true. This method has nothing to do with the the private browsing window
> loaded. It has been written for content documents.

I have added a dump for the win.document.title to controller.js - waitForPageLoad before this line: https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L1330

I have attached the log for when the test is failing and I will attach one for when it passes.

When the test fails, the waitFor stops when it found Private Browsing, but when it passes it waits for Flash Cookie. So, I thought that it is related to the waitForPageLoad.

Comment 36

[Daniela Petrovici]

Attachment: log pass

Comment 37

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

isLoaded() is a totally different thing. It works with the internal window id. The failure log is not helpful. You will have to run it with --show-errors. So I can't tell anything at the moment.

Comment 38

[Daniela Petrovici]

I have added the information to bug 851006 and marked this issue dependent on it.

Comment 39

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

Ok, so lets keep the waitForPageLoad() in the code for now and add a comment above as usual to indicate why it's necessary.

Comment 40

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 724441 [details] [diff] [review]
patch v1.5

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

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +10,5 @@
> +var tabs = require("../../../lib/tabs");
> +
> +const COOKIE_VALUE = "cookieValue";
> +
> +const TEST_FOLDER = "http://mozqa.com/data/firefox/plugins/flash/cookies/"

Please add semicolon here.

@@ +25,5 @@
> +function teardownModule(aModule) {
> +  aModule.pbWindow.close();
> +  clearPluginData();
> +}
> +

Could you add a comment for this function?

@@ +28,5 @@
> +}
> +
> +function testCheckFlashCookie() {
> +  pbWindow.open(controller);
> +  pbWindow.controller.waitForPageLoad();

Comment here as Henrik suggested so we know when we need to change it.

@@ +67,5 @@
> +}
> +
> +function clearPluginData() {
> +  // Clear plugin data.
> +  const phInterface = Ci.nsIPluginHost;

We should either rename it to PHINTERFACE giving that out contributor Jonathan is updating all constants now, or we can remove it and use it directly, as we do with Services.* in our libraries.
I'd go with the second choice.

@@ +73,5 @@
> +
> +  var ph = Cc["@mozilla.org/plugin/host;1"].getService(phInterface);
> +  var tags = ph.getPluginTags();
> +
> +  for (var i = 0; i < tags.length; i++) {

Could we better use forEach here?

Comment 41

[Daniela Petrovici]

Attachment: patch v1.6

Modified the patch based on review.

Reports are:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c585107d12f7
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c585107d3ba9
MAC: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c585107db2da

Comment 42

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

Comment on attachment 724946 [details] [diff] [review]
patch v1.6

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

Some comments here I would like to see fixed. Stealing review from Andreea.

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +9,5 @@
> +
> +const COOKIE_VALUE = "cookieValue";
> +
> +const TEST_FOLDER = "http://mozqa.com/data/firefox/plugins/flash/cookies/";
> +const TEST_PAGE = TEST_FOLDER + "flash_cookie.html";

Please do not make those strange constructs. Make it a single constant with the name TEST_URL and put it in as the first constant.

@@ +32,5 @@
> +function testCheckFlashCookie() {
> +  pbWindow.open(controller);
> +
> +  // XXX: Bug 847991
> +  pbWindow.controller.waitForPageLoad();

This comment is a no-op. Please add a description.

@@ +69,5 @@
> +  pbWindow.controller.click(clearCookie);
> +  assert.equal(resultFieldPB.getNode().value, "undefined", "Flash cookie was cleared");
> +}
> +
> +function clearPluginData() {

This should be not part of the test, but end-up in a library module. Further it needs a jsdoc comment.

Comment 43

[Daniela Petrovici]

Attachment: patch v1.7

Modified patch based on review

Comment 44

[Daniela Petrovici]

Attachment: patch v1.8

Made a mistake while modifying the page - the private-browsing library should have been modified as part of bug 851006

Comment 45

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 725303 [details] [diff] [review]
patch v1.8

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

Tests nice. Pushed to default:
http://hg.mozilla.org/qa/mozmill-tests/rev/22c6204eeb39 (default)

Comment 46

[Daniela Petrovici]

Attachment: patch v1.0 for Aurora

Patch does not apply cleanly on Aurora. I have added a new patch. Reports for:
- Linux: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510a6ad2a
- MAC: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510afc645
- Windows: http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510afefd8

Comment 47

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 725404 [details] [diff] [review]
patch v1.0 for Aurora

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

I see we caught a failure on OS X 10.6.8, giving that is just one, I would like to see a follow up, maybe we should change the assert.equal to waitFor? Basically we had nothing in the "Current value" input field.
http://mozmill-ci.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510bbc863

Comment 48

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

I had to backout this patch from default due to a lot of test failures:
http://hg.mozilla.org/qa/mozmill-tests/rev/70a9aad89905

Comment 49

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #48)
> I had to backout this patch from default due to a lot of test failures:
> http://hg.mozilla.org/qa/mozmill-tests/rev/70a9aad89905

This happened only on MAC 10.6.8 and it is because Flash was disabled on our 10.6 machines per bug 838134. Based on the last comment in the bug, Flash will not get re-enabled on the 10.6 machines. Should we disable the test on MAC 10.6.8?

Comment 50

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

(In reply to Daniela Petrovici from comment #49)
> This happened only on MAC 10.6.8 and it is because Flash was disabled on our
> 10.6 machines per bug 838134. Based on the last comment in the bug, Flash
> will not get re-enabled on the 10.6 machines. Should we disable the test on
> MAC 10.6.8?

Yes please.

Comment 51

[Daniela Petrovici]

Attachment: patch v1.9 for default branch

Created patch to disable the test on MAC 10.6.8.

Reports on MAC OS 10.6.8:
http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510f69c84
Report on MAC OS 10.7.5:
http://mozmill-crowd.blargon7.com/#/functional/report/2a6536e9db9f5f44ed48c58510f4db00

Comment 52

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 726179 [details] [diff] [review]
patch v1.9 for default branch

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

Pushed back on default, this time skipping the test on 10.6.8 os x machines. 
The test did not failed on other platforms over the weekend.

http://hg.mozilla.org/qa/mozmill-tests/rev/a86c39572310 (default)

Comment 53

[Daniela Petrovici]

Attachment: patch v1.0 for Aurora

The patch for default does not apply cleanly on Aurora, so I have modified it for this branch. The reports are:
MAC OS 10.6.8: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad60548c2
MAC OS 10.7.5: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad60bea27
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad60bc544
Windows: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad60c0157

Comment 54

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 727090 [details] [diff] [review]
patch v1.0 for Aurora

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7aa7b8f6cfdc (aurora)

We're done here now. Thanks Daniela!

Comment 55

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

Please do not implement platform specific skip logic until it is really necessary. Please backout both patches from default and aurora and lets get this fixed correctly, similar to the other plugin tests we have in our repository. Those check if Flash is available and enabled and perform the test only under such a condition. Keep in mind that 10.6 is still supported and other users might want to run this test. It's not only our CI we care about. Thanks.

Comment 56

[Andreea Matei [:AndreeaMatei]]

You're right, haven't thought of that.
Backed out:
http://hg.mozilla.org/qa/mozmill-tests/rev/5bac1efdfa01 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/ca996a686b45 (aurora)

Comment 57

[Daniela Petrovici]

Attachment: patch v1.10 for default

Sorry for not thinking about this scenario. I have modified the patch to check for Flash plugin being installed.

Comment 58

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 727181 [details] [diff] [review]
patch v1.10 for default

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

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +23,5 @@
> +    if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name.indexOf("Flash") > 0)
> +      return true;
> +  });
> +
> +  if (isFlashActive[0] !== true) {

Could we use if(!isFlashActive[0]) better?

Comment 59

[Daniela Petrovici]

Attachment: patch v1.11 for default

Made changes based on review

Comment 60

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 727666 [details] [diff] [review]
patch v1.11 for default

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/76874359593f (default)

Comment 61

[Daniela Petrovici]

Attachment: patch v1.3 for Aurora

Created the patch for Aurora. Reports are:
http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad67d2044 - Flash not enabled
http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad67f5966 - Flash enabled

Comment 62

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

Comment on attachment 727666 [details] [diff] [review]
patch v1.11 for default

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

::: tests/functional/testPrivateBrowsing/testFlashCookie.js
@@ +19,5 @@
> +  aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> +
> +  // Skip test if we don't have Flash plugin enabled
> +  var isFlashActive = addons.getInstalledAddons(function (aAddon) {
> +    if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name.indexOf("Flash") > 0)

Why are we using a part of the name of the plugin to determine it? This can cause troubles if other plugins are installed which also contain 'Flash' in their name. Can't we use an id? Don't have plugins an id?

@@ +24,5 @@
> +      return true;
> +  });
> +
> +  if (!isFlashActive[0]) {
> +    testCheckFlashCookie.__force_skip__ = "No enabled Flash plugin detected";

Why aren't we skipping the teardownModule here? It will not fail at the moment but will once we call code which depends on changes made in setupModule or the test itself.

Comment 63

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

Comment on attachment 728236 [details] [diff] [review]
patch v1.3 for Aurora

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

Lets get the missing pieces on default addressed first before we backport this patch. Thanks.

Comment 64

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #62)
> Comment on attachment 727666 [details] [diff] [review]
> patch v1.11 for default
> 
> Review of attachment 727666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: tests/functional/testPrivateBrowsing/testFlashCookie.js
> @@ +19,5 @@
> > +  aModule.pbWindow = new privateBrowsing.PrivateBrowsingWindow();
> > +
> > +  // Skip test if we don't have Flash plugin enabled
> > +  var isFlashActive = addons.getInstalledAddons(function (aAddon) {
> > +    if (aAddon.isActive && aAddon.type === "plugin" && aAddon.name.indexOf("Flash") > 0)
> 
> Why are we using a part of the name of the plugin to determine it? This can
> cause troubles if other plugins are installed which also contain 'Flash' in
> their name. Can't we use an id? Don't have plugins an id?

I can use the ID. Should I use a constant and add the unique ID to it and use it in the test?
The value of the ID will be: {3050f584-f0fb-1a0d-6e4f-c4c1d0f1ecec}

> @@ +24,5 @@
> > +      return true;
> > +  });
> > +
> > +  if (!isFlashActive[0]) {
> > +    testCheckFlashCookie.__force_skip__ = "No enabled Flash plugin detected";
> 
> Why aren't we skipping the teardownModule here? It will not fail at the
> moment but will once we call code which depends on changes made in
> setupModule or the test itself.

Comment 65

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

(In reply to Daniela Petrovici from comment #64)
> I can use the ID. Should I use a constant and add the unique ID to it and
> use it in the test?
> The value of the ID will be: {3050f584-f0fb-1a0d-6e4f-c4c1d0f1ecec}

I'm sure that our coding styles are clear about that. So I don't understand this question.

Comment 66

[Daniela Petrovici]

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #65)
> (In reply to Daniela Petrovici from comment #64)
> > I can use the ID. Should I use a constant and add the unique ID to it and
> > use it in the test?
> > The value of the ID will be: {3050f584-f0fb-1a0d-6e4f-c4c1d0f1ecec}
> 
> I'm sure that our coding styles are clear about that. So I don't understand
> this question.

I am sorry the question was not clear. I wanted to make sure if we need to directly use the plugin ID in the test since they appear to be platform dependent.

I checked and the plugin ID is different between Windows/MAC and Linux. This is the plugin name for:
- Windows 8 and MAC OS 10.8.2 - {b0417cfa-8315-9282-d1ae-629120bf126c}
- Linux 12.04 - {3050f584-f0fb-1a0d-6e4f-c4c1d0f1ecec}

So, do we still want to add all values to the test in order to use the plugin ID?

Comment 67

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

Is the id dependent on the version of Flash? I know that on Linux the current version is still 11.2.

Comment 68

[Daniela Petrovici]

On MAC OS and Windows it is version is 11.6 and Linux is 11.2. So, the ID is dependent on the version of Flash.

Comment 69

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

Have you tested 11.2 on Windows or Mac to be sure about your last statement?

Comment 70

[Daniela Petrovici]

I have tested on Windows 8 x86 with Flash player version 11.2.202.228 taken from: http://fpdownload.macromedia.com/get/flashplayer/installers/archive/fp_11.2.202.228_archive.zip
The plugin ID is: {3050f584-f0fb-1a0d-6e4f-c4c1d0f1ecec} - the same as on Linux with the 11.2.202.275 Flash player version

Comment 71

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

Alright. So in all of our tests which are based on Flash we should use "Shockwave Flash" instead. Is that the full name? It would be good to not have to fallback to indexOf(). We could fix that along all the other affected tests in a follow-up.

Comment 72

[Daniela Petrovici]

Attachment: follow up patch for default

(In reply to Henrik Skupin (:whimboo) [away 03/15 - 03/22] from comment #71)
> Alright. So in all of our tests which are based on Flash we should use
> "Shockwave Flash" instead. Is that the full name? It would be good to not
> have to fallback to indexOf(). We could fix that along all the other
> affected tests in a follow-up.
The full name of the plugin is "Shockwave Flash". I have removed "indexOf" and used the full name to verify the plugin's existance in this test and in the endurance tests where this happened also.

Also modified to skip the teardown per the previous review in case Flash is not enabled. 

Reports:
Functional - with Flash on: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6f32d9f
Functional - with Flash off: http://mozmill-crowd.blargon7.com/#/functional/report/db924715258381629c26ddbad6f31d7a

Endurance - with Flash on: http://mozmill-crowd.blargon7.com/#/endurance/report/677f05f4f4af7748bfaf0197ee03d459
Endurance - with Flash off: http://mozmill-crowd.blargon7.com/#/endurance/report/677f05f4f4af7748bfaf0197ee03ad67

Comment 73

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 729534 [details] [diff] [review]
follow up patch for default

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

Landed follow-up on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/da97754b927a

Comment 74

[Daniela Petrovici]

Attachment: patch v2.0 for Aurora

Created patch for Aurora. Also updated the endurance tests for Flash in the same patch

Reports:
Functional: http://mozmill-crowd.blargon7.com/#/functional/report/25ad365ca7bcf4905e9b700b4f7e37e5 - with flash
http://mozmill-crowd.blargon7.com/#/functional/report/25ad365ca7bcf4905e9b700b4f7ea232 - without Flash
Endurance:
http://mozmill-crowd.blargon7.com/#/endurance/report/25ad365ca7bcf4905e9b700b4f806e9b - with Flash
http://mozmill-crowd.blargon7.com/#/endurance/report/25ad365ca7bcf4905e9b700b4f81ef33 - without Flash

Comment 75

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 731821 [details] [diff] [review]
patch v2.0 for Aurora

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

http://hg.mozilla.org/qa/mozmill-tests/rev/68fb9da81555 (aurora)

Now we're done :)
Thanks!