989922 - Test failure "Element.waitForElement(): Element 'ID: errorTitleText' has been found" in testSecurity/testSSLDisabledErrorPage.js

Status: RESOLVED FIXED

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

Description

[Andreea Matei [:AndreeaMatei]]

Started failing yesterday, affecting all platforms. Must be a regression, something changed on the remote page we're using.

Andrei is investigating this.

Comment 1

[Andrei Eftimie]

Attachment: 1.patch

It appears that the previously used domain https://mail.mozilla.org has been updated to use TLS 1.2

Our test enforces the use of TLS 1.2 through the set prefs and needs an older version on a webpage to assert the correctness of the error page.

This fix changes the domain to our own http://mozqa.com where we have TLS 1.1 where we correctly show the error. This will also make this test more future proof since we have direct control over mozqa.com

This patch applies to and fixes the issue across branches.

Comment 2

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8399337 [details] [diff] [review]
1.patch

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

http://hg.mozilla.org/qa/mozmill-tests/rev/b917b8bfa832 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/5980b9450193 (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/b1609bcfb3fe (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/d4d73259e5fa (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/51055dec5e82 (esr24)

Thanks for the quick fix!

Comment 3

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

I consider this as a workaround given that we also want to upgrade the default SSL vhost to a later TLS version. So Andrei, please file a new bug so that we can update the URL to a dedicated vhost on mozqa.com. It would still be blocked by bug 891288.

Comment 4

[Andreea Matei [:AndreeaMatei]]

This still fails, some test in the folder that using mozqa.com must be blocking us to get the error page when ran in a testrun. 

We're skipping now and further investigate.

Comment 5

[Andrei Eftimie]

Attachment: skip.patch

Lets skip it for now.
This skip patch applies to default, mozilla-aurora, mozilla-beta.
I'll shortly add a patch for mozilla-release and mozilla-esr24 where we have conflicts in the manifest file.

This still fails because we fail to properly clean up in a previous test where mozqa.com is added as a trusted domain (or something along these lines). I'll come with exact details shortly.

This is my bad: I didn't do a full testrun because the changes were minimal, I should have seen this before.

Comment 6

[Andrei Eftimie]

Attachment: skip_release.patch

Comment 7

[Andrei Eftimie]

Attachment: skip_esr24.patch

Comment 8

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8399384 [details] [diff] [review]
skip.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/2abdb8d9a869 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/999a8cce947b (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/020aa63e40f3 (beta)

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8399388 [details] [diff] [review]
skip_release.patch

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

Disabled:
http://hg.mozilla.org/qa/mozmill-tests/rev/07d5717a7bee (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/18b5fd6efc6b (esr24)

Comment 10

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

As suggested earlier lets make sure we use a proper subdomain for this test. I think it shouldn't be too hard to get bug 891288 fixed.

Comment 11

[Andrei Eftimie]

I can reproduce the issue manually. Cached items work past the certificate check. I'm not sure but this might be a bug in Firefox.

Steps to reproduce manually:

1) Open Firefox with a new profile
2) Visit "https://mozqa.com" (this uses TLS 1.1)
3) Open "about:config" and change "security.tls.version.min" to "3" (this will only accept TLS 1.2
4) Open "https://mozqa.com"

Expected results:
"Secure Connection Failed" error page shown

Actual results:
The page is rendered

Only in case of a reload the error page is displayed.

Even after the error page is displayed, close the tab and open https://mozqa.com again, it will again be rendered.

Cached pages bypass the TLS version verification. I'm not sure if this is in any way exploitable, but it does look like a security issue.

Comment 12

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

Well, that's why the cache exists so you don't have to request the page again. Given that the cache has a short lifetime (?) it shouldn't be that problematic. Best would be to ask someone from the networking team for info.

Comment 13

[Andrei Eftimie]

Patrick would you know if the steps described in comment 11 might be a security bug?

Comment 14

[Patrick McManus [:mcmanus]]

Hi all - good news about mail.mozilla.com being upgraded! :)

see bug 660749.. we've basically decided that the transport issues for how the document made it into the cache don't imapct the cache entries lifetime. (same issue as if a cert was revoked while a document served with it lives in your cache - your cache is still valid.)

660749 is still open because it interacts with the ability to manage your cert exception list.

Comment 15

[Andrei Eftimie]

Attachment: fix.patch

Not sure why it was needed to reassign the bug.
Here's an easy fix.

I didn't went through initially with something like this because in comment 10 it was suggested to have a proper fix and have different TLS versions available on different subdomains (in bug 891288).

Here's the report:
http://mozmill-crowd.blargon7.com/#/remote/report/7ab760e27012969ae02e3b9e41774c4d

This patch is for Nightly, I have some conflicts in the manifest file for the other branches. I'll come shortly with backpors.

Comment 16

[Andrei Eftimie]

Attachment: fix_aurora.patch

Because of the manifest file I have a different patch for each branch :(

Comment 17

[Andrei Eftimie]

Attachment: fix_beta.patch

Comment 18

[Andrei Eftimie]

Attachment: fix_release.patch

Comment 19

[Andrei Eftimie]

Attachment: fix_esr24.patch

Comment 20

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

(In reply to Andrei Eftimie from comment #15)
> I didn't went through initially with something like this because in comment
> 10 it was suggested to have a proper fix and have different TLS versions
> available on different subdomains (in bug 891288).

That was before we have known the possible conflicts in updating apache on mozqa.com. So dependencies would have to be re-evaluated.

Comment 21

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

Comment on attachment 8403804 [details] [diff] [review]
fix_aurora.patch

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

::: firefox/tests/remote/testSecurity/testSSLDisabledErrorPage.js
@@ +9,5 @@
>  var prefs = require("../../../lib/prefs");
>  var tabs = require("../../../lib/tabs");
>  var utils = require("../../../lib/utils");
>  
> +const TEST_DATA = "https://www.mozqa.com";

Wouldn't the test still fail if another test before this one, opens this page?

Comment 22

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #21)
> Comment on attachment 8403804 [details] [diff] [review]
> fix_aurora.patch
> 
> Review of attachment 8403804 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/tests/remote/testSecurity/testSSLDisabledErrorPage.js
> @@ +9,5 @@
> >  var prefs = require("../../../lib/prefs");
> >  var tabs = require("../../../lib/tabs");
> >  var utils = require("../../../lib/utils");
> >  
> > +const TEST_DATA = "https://www.mozqa.com";
> 
> Wouldn't the test still fail if another test before this one, opens this
> page?

Yep, it will.

I've initially tried using evictEntries[1] both in this test's setupModule, and in the previous test teardownModule with no effect. Another easy solution might be to clean the profile (eg. stopApplication(true)) but I don't think we want that either. Or make this test run _before_ others.

At the moment this seems the best solution.

[1] https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/nsICacheService#evictEntries()

Comment 23

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

I'm not sure if certificates are getting stored in that cache. You might want to ask Patrick again, how to get rid of those entries.

Comment 24

[Andreea Matei [:AndreeaMatei]]

Not reviewing this fix yet since we want to look further to the other solution. If that won't be possible, we might want to consider it.

Comment 25

[Andrei Eftimie]

Attachment: fix 2

I've created a new function in utils called sanitize based on http://dxr.mozilla.org/mozilla-central/source/browser/base/content/sanitize.js

Calling the Sanitizer without any other arguments clears everything.
We can later improve this method so that it accepts certain arguments to only clear specific data.

This clears the cache and testSSLDisabledErrorPage.js correctly loads the SSL error instead of loading the page from cache.

Comment 26

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

I think what we simply would need here is:

        var cache = Cc["@mozilla.org/netwerk/cache-storage-service;1"].
                    getService(Ci.nsICacheStorageService);
        try {
          // Cache doesn't consult timespan, nor does it have the
          // facility for timespan-based eviction.  Wipe it.
          cache.clear();
        } catch(er) {}

Comment 27

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #26)
I've tried that and it's not enough. I haven't tried all of them. It might be that we need cashing + placesHistory (or maybe another combination).

Sanitize does everything. I'm not completely sure of the implementation I've suggested, but I feel something like this might be of good use in utils.js

Comment 28

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

(In reply to Andrei Eftimie from comment #27)
> (In reply to Henrik Skupin (:whimboo) from comment #26)
> I've tried that and it's not enough. I haven't tried all of them. It might
> be that we need cashing + placesHistory (or maybe another combination).
> 
> Sanitize does everything. I'm not completely sure of the implementation I've
> suggested, but I feel something like this might be of good use in utils.js

Patrick or Daniel might know more here.

Comment 29

[Patrick McManus [:mcmanus]]

I have no idea what the question that generated the needinfo is :)

Comment 30

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

I'm not Daniel.

Comment 31

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

Sorry David, I miss-typed your name. So what we are looking for here is how to remove cached certificates. Which feature of the sanitizer has to be used to actually do that?

Comment 32

[Andrei Eftimie]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> Sorry David, I miss-typed your name. So what we are looking for here is how
> to remove cached certificates. Which feature of the sanitizer has to be used
> to actually do that?

From comment 14 I understand that this has nothing to do with certificates (or cached certs) Henrik, a page delivered from cache will bypass those checks. If I clean userdata its all working fine.

We *could* try clearing only certain aspects of it (like cache, pagehistory, etc), I'm not sure its worth it right now.

I've already proposed 2 solutions:
1) We visit a URL that wasn't previously visited (so not in the cache)
2) We sanitize all user data in setupModule, this way we're sure to _not_ have visited the page before.

Neither of these have anything to do with the current affected test.

Comment 33

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Henrik Skupin (:whimboo) from comment #31)
> Sorry David, I miss-typed your name. So what we are looking for here is how
> to remove cached certificates. Which feature of the sanitizer has to be used
> to actually do that?

Oh, I see - I thought you were looking for someone else :)
Like Andrei said, I think it's not so much the certificates that matter but the cache - if I shift-refresh the page for step 4 of the STR in comment 11, it works for me as expected (this clears the cache for that page, if I understand correctly).

Comment 34

[Patrick McManus [:mcmanus]]

you can either make your query with nsIRequest::LOAD_BYPASS_CACHE or trying and clear out the http disk cache as you are doing.. if the conflicting information is stored somewhere other than the http cache I would have to debug it too to find out where.

Comment 35

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8406120 [details] [diff] [review]
fix 2

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

This looks good to me. Henrik, are we in agreement to land this fix?

Comment 36

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

Comment on attachment 8406120 [details] [diff] [review]
fix 2

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

::: firefox/lib/utils.js
@@ +15,5 @@
> +let tempScope = {}
> +Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader)
> +                                           .loadSubScript("chrome://browser/content/sanitize.js",
> +                                                          tempScope);
> +let Sanitizer = tempScope.Sanitizer;

I don't see why all that has to live in the global namespace.

@@ +516,5 @@
> + */
> +function sanitize() {
> +  var s = new Sanitizer();
> +  s.prefDomain = "privacy.cpd.";
> +  s.sanitize();

This method is using promises. So it's not synchronous as you 
are expecting here.

Also I would still like to know which specific settings we have to clear to get it working.

Comment 37

[Andrei Eftimie]

Attachment: fix3.patch

(In reply to Henrik Skupin (:whimboo) from comment #36)
> Comment on attachment 8406120 [details] [diff] [review]
> fix 2
> 
> Review of attachment 8406120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: firefox/lib/utils.js
> @@ +15,5 @@
> > +let tempScope = {}
> > +Cc["@mozilla.org/moz/jssubscript-loader;1"].getService(Ci.mozIJSSubScriptLoader)
> > +                                           .loadSubScript("chrome://browser/content/sanitize.js",
> > +                                                          tempScope);
> > +let Sanitizer = tempScope.Sanitizer;
> 
> I don't see why all that has to live in the global namespace.

Moved them in the sanitize method.

> @@ +516,5 @@
> > + */
> > +function sanitize() {
> > +  var s = new Sanitizer();
> > +  s.prefDomain = "privacy.cpd.";
> > +  s.sanitize();
> 
> This method is using promises. So it's not synchronous as you 
> are expecting here.

Right, I updated the code to wait for the promise to resolve.

> Also I would still like to know which specific settings we have to clear to
> get it working.

It seems we only need to clear the current session for this particular issue.

==
I've left the sanitize method to be able to clear them all, you only need to pass in the spec object and set which items you want cleared.

For this I also included the extend method (initially worked on in bug 799149) so we can easily merge the spec object properties.

I've now left it with by default to not clear anything, each item must be specified individually. Not sure if we should also have an `all` option.

Report:
http://mozmill-crowd.blargon7.com/#/remote/report/c90eeeb0a5511695efc7462553233bfe

Comment 38

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8410291 [details] [diff] [review]
fix3.patch

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

I'd vote for an 'all' option or something, for me the call without any spec mentioned should clear everything. But that would mean each set to true by default and when you want to clear just "cookies", you have to set all of them to false (except cookies). That's not something I would like, the 'all' option seems cleaner.

::: firefox/lib/utils.js
@@ +536,5 @@
> + * @param {boolean} [aSpec.formdata=false]
> + * @param {boolean} [aSpec.downloads=false]
> + * @param {boolean} [aSpec.passwords=false]
> + * @param {boolean} [aSpec.sessions=false]
> + * @param {boolean} [aSpec.siteSettings=false]

Please sort them alphabetically.

@@ +549,5 @@
> +    downloads: false,
> +    passwords: false,
> +    sessions: false,
> +    siteSettings: false
> +  }, aSpec);

Same here.

@@ +555,5 @@
> +  // Load the sanitize script
> +  var tempScope = {}
> +  Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +    .getService(Ci.mozIJSSubScriptLoader)
> +    .loadSubScript("chrome://browser/content/sanitize.js",

The dot goes at the end of the previous line and the rest aligned to Cc:
https://developer.mozilla.org/en-US/docs/Mozilla/QA/Mozmill_tests/Mozmill_Style_Guide#Components

Comment 39

[Andrei Eftimie]

Attachment: fix3.1.patch

Fixed issues mentioned in the review.

> I'd vote for an 'all' option or something, for me the call without any spec
> mentioned should clear everything. But that would mean each set to true by
> default and when you want to clear just "cookies", you have to set all of
> them to false (except cookies). That's not something I would like, the 'all'
> option seems cleaner.

I've given this some thought and my solution is the following:

A call to sanitize() will clear _everything_.
If you want specific items cleared call sanitize({ history: true }). This will only clear the history entries.

Henrik, I'd like your thoughts on these changes. Thanks.

And a report: http://mozmill-crowd.blargon7.com/#/remote/report/c90eeeb0a5511695efc74625537c2a7a

Comment 40

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

Comment on attachment 8410826 [details] [diff] [review]
fix3.1.patch

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

::: firefox/lib/utils.js
@@ +253,5 @@
>    clipboard.copyString("");
>  }
>  
>  /**
> + * Returns an extended object of all objects pased-in as parameters

nit: pased-in?

Also you create an extended new object only for properties but not methods, right? That should be noted.

@@ +254,5 @@
>  }
>  
>  /**
> + * Returns an extended object of all objects pased-in as parameters
> + *

Where is @params?

@@ +257,5 @@
> + * Returns an extended object of all objects pased-in as parameters
> + *
> + * @return {Object} The extended object
> + */
> +function extend() {

I would like to see a better name here like mergeObjects. Also I would still list the needed parameters in the function head.

@@ +262,5 @@
> +  var object = {};
> +
> +  Array.prototype.slice.call(arguments, 0).map(aSource => {
> +    if (aSource) {
> +      for (var prop in aSource) {

Shouldn't we better use for of here?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...of

@@ +527,5 @@
> + * This clears: cache, cookies, offlineApps, history, formdata, downloads,
> + * passwords, sessions, siteSettings
> + *
> + * Usage:
> + * sanitize(); // Will clean all user data

nit: clear

@@ +536,5 @@
> + *
> + * @param {object} [aSpec]
> + *        Configuration object, holds options for which items to sanitize
> + *        If aSpec is undefined sanitize will clear ALL options mentioned below
> + *        Otherwise only the specific items set to true will be cleared.

I would reduce the last 2 lines to: 'If undefined all data will be cleared'.

@@ +537,5 @@
> + * @param {object} [aSpec]
> + *        Configuration object, holds options for which items to sanitize
> + *        If aSpec is undefined sanitize will clear ALL options mentioned below
> + *        Otherwise only the specific items set to true will be cleared.
> + * @param {boolean} [aSpec.cache=false]

For a good jsdoc documentation you will have to add a description for all of those properties.

@@ +561,5 @@
> +    siteSettings: false
> +  }, aSpec);
> +
> +  // Load the sanitize script
> +  var tempScope = {}

nit: missing semicolon.

@@ +564,5 @@
> +  // Load the sanitize script
> +  var tempScope = {}
> +  Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +  getService(Ci.mozIJSSubScriptLoader).
> +  loadSubScript("chrome://browser/content/sanitize.js", tempScope);

I thought that I have mentioned that this notation with the dot at the end is totally misleading. Here you think the new line has a new function call. If that is in our coding styles we should update that to be conform with the general Mozilla style, which is:

Cc["@mozilla.org/moz/jssubscript-loader;1"]
.getService(Ci.mozIJSSubScriptLoader)
.loadSubScript("chrome://browser/content/sanitize.js", tempScope);

@@ +566,5 @@
> +  Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +  getService(Ci.mozIJSSubScriptLoader).
> +  loadSubScript("chrome://browser/content/sanitize.js", tempScope);
> +
> +  var Sanitizer = tempScope.Sanitizer;

I don't see why we need the variable when it is used only once.

@@ +574,5 @@
> +  s.prefDomain = "privacy.cpd.";
> +  var itemPrefs = Services.prefs.getBranch(s.prefDomain);
> +
> +  // Apply options for what to sanitize
> +  for (var pref in spec) {

for of... here too?

@@ +579,5 @@
> +    itemPrefs.setBoolPref(pref, spec[pref]);
> +  };
> +
> +  // Sanitize and wait for the promise to resolve
> +  var sanitizeHasFinished = false;

Too long variable. Just call it `finished`.

@@ +582,5 @@
> +  // Sanitize and wait for the promise to resolve
> +  var sanitizeHasFinished = false;
> +  s.sanitize().then(() => {
> +    sanitizeHasFinished = true;
> +  });

Single line please. Also add a second parameter to the call to then() for the failing case.

@@ +588,5 @@
> +
> +  // Restore prefs to default
> +  for (var pref in spec) {
> +    itemPrefs.clearUserPref(pref);
> +  };

In case of a failure above, the preferences set before will not be cleaned-up.

::: firefox/tests/remote/testSecurity/testSSLDisabledErrorPage.js
@@ +23,5 @@
>  
>    tabs.closeAllTabs(aModule.controller);
> +  utils.sanitize({
> +    sessions: true
> +  });

Single line please.

Comment 41

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: fix3.2.patch

(In reply to Henrik Skupin (:whimboo) from comment #40)
> Also you create an extended new object only for properties but not methods,
> right? That should be noted.
This extends the methods too.
> I would like to see a better name here like mergeObjects. Also I would still
> list the needed parameters in the function head.
We couldn't list the needed parameters as we could merge any number of objects, I know this looks bad but it's the way is implemented in most libraries.
For documentation I added:
>@param [arguments] All object's gave as parameters for extending

> > +  Array.prototype.slice.call(arguments, 0).map(aSource => {
> > +    if (aSource) {
> > +      for (var prop in aSource) {
> 
> Shouldn't we better use for of here?
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/
> for...of
This works only for arrays at the moment when called on an object(like arguments) it throws:
TypeError: a['@@iterator'] is not a function

Comment 42

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8411925 [details] [diff] [review]
fix3.2.patch

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

::: firefox/lib/utils.js
@@ +473,5 @@
>    return visible;
>  }
>  
>  /**
> + * Returns an extended object of all objects gave as parameters

nit: given

@@ +475,5 @@
>  
>  /**
> + * Returns an extended object of all objects gave as parameters
> + *
> + * @param [arguments] All object's gave as parameters for extending

nit: objects, given

@@ +477,5 @@
> + * Returns an extended object of all objects gave as parameters
> + *
> + * @param [arguments] All object's gave as parameters for extending
> + *
> + * @return {Object} The extended object

Type should be with lowercase please.

@@ +555,5 @@
> + *        If true will remove persisted passwords
> + * @param {boolean} [aSpec.sessions=false]
> + *        If true will clear session storage
> + * @param {boolean} [aSpec.siteSettings=false]
> + *        If true will clear site Settings

Please keep consistency in the descriptions "will be cleared" vs "will remove/will clear"

@@ +574,5 @@
> +  // Load the sanitize script
> +  var tempScope = {};
> +  Cc["@mozilla.org/moz/jssubscript-loader;1"]
> +  .getService(Ci.mozIJSSubScriptLoader)
> +  .loadSubScript("chrome://browser/content/sanitize.js", tempScope);

Please file a bug to update the cases with the dot at the beginning then, I'll update out the style guide.

Comment 43

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: fix3.3.patch

(In reply to Andreea Matei [:AndreeaMatei] from comment #42)
> Please file a bug to update the cases with the dot at the beginning then,
> I'll update out the style guide.
bug 1001392
Thanks for review.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8412588 [details] [diff] [review]
fix3.3.patch

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

Looks good now.

Comment 45

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

Comment on attachment 8412588 [details] [diff] [review]
fix3.3.patch

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

::: firefox/lib/utils.js
@@ +473,5 @@
>    return visible;
>  }
>  
>  /**
> + * Returns an extended object of all objects given as parameters

We return a new object, extended by properties and methods for all passed in objects.

@@ +475,5 @@
>  
>  /**
> + * Returns an extended object of all objects given as parameters
> + *
> + * @param [arguments] All objects given as parameters for extending

This is not an allowed syntax for jsdoc. An array is defined as {name[]}

@@ +477,5 @@
> + * Returns an extended object of all objects given as parameters
> + *
> + * @param [arguments] All objects given as parameters for extending
> + *
> + * @return {object} The extended object

nit: returns

@@ +479,5 @@
> + * @param [arguments] All objects given as parameters for extending
> + *
> + * @return {object} The extended object
> + */
> +function mergeObjects() {

You might want to add the parameters as comment to the function head like `/* obj1, ob2, ... */`

@@ +525,5 @@
>  }
>  
>  /**
> + * Sanitize user data
> + * This clear: cache, cookies, offlineApps, history, formdata, downloads,

nit: `this clears`. I would also remove the colon and continue in the first line.

@@ +533,5 @@
> + * sanitize(); // Will clear all user data
> + * sanitize({
> + *   downloads: true,
> + *   history: true
> + * }); // Will clear only downloads and history

I would reduce this example to e.g. downloads only, so it can be a single line.

@@ +536,5 @@
> + *   history: true
> + * }); // Will clear only downloads and history
> + *
> + * @param {object} [aSpec]
> + *        Configuration object, holds options for which items to sanitize

nit: Missing final point.

@@ +555,5 @@
> + *        If true persisted passwords will be cleared
> + * @param {boolean} [aSpec.sessions=false]
> + *        If true session storage will be cleared
> + * @param {boolean} [aSpec.siteSettings=false]
> + *        If true site Settings will be cleared

'settings'

@@ +568,5 @@
> +    offlineApps: false,
> +    passwords: false,
> +    sessions: false,
> +    siteSettings: false
> +  }, aSpec);

While inspecting this deeper, I don't really see value for mergeObjects here. All the supported properties we will overwrite anyway. So just define a new object here. The mergeObjects() method you can move over to the patch of the l10n test.

@@ +589,5 @@
> +
> +  try {
> +    // Sanitize and wait for the promise to resolve
> +    var finished = false;
> +    s.sanitize().then(() => { finished = true; },

For better readability please move the function bodies to its own new line by indenting 2 chars.

@@ +590,5 @@
> +  try {
> +    // Sanitize and wait for the promise to resolve
> +    var finished = false;
> +    s.sanitize().then(() => { finished = true; },
> +                      aReason => { throw new Error(aReason) });

The parameter should not be a message but an error instance. So do not recreate an Error object here. This will change the stack information. Instead just 'throw ex'.

Comment 46

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.0

I removed the merge method as requested and I will add it in l10n bug.
Thanks 
http://mozmill-crowd.blargon7.com/#/remote/report/0bd22723d1fdbb446269f765b9819d62
http://mozmill-crowd.blargon7.com/#/remote/report/0bd22723d1fdbb446269f765b981564f
http://mozmill-crowd.blargon7.com/#/remote/report/0bd22723d1fdbb446269f765b9825dcb

Comment 47

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

Comment on attachment 8418004 [details] [diff] [review]
patch_v4.0

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

Not sure why this didn't went through review by Andreea and Andrei first. But here my comments. Please fix the nits and you have my r+.

::: firefox/lib/utils.js
@@ +567,5 @@
> +    var finished = false;
> +    s.sanitize().then(() => {
> +                        finished = true;
> +                      },
> +                      aException => { throw aException });

We still have 2 char indentation in our style guide. Also I would call the parameter `aError`. So it should read as:

s.sanitize().then(() => {
  finished = true;
}, aError => { 
  throw aError
});

Comment 48

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.1

Thanks for review Henrik.

Comment 49

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8418687 [details] [diff] [review]
patch_v4.1

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

With that change I can land this.

::: firefox/lib/utils.js
@@ +567,5 @@
> +    var finished = false;
> +    s.sanitize().then(() => {
> +      finished = true;
> +    }, aError => {
> +      throw aError

Just add a semicolon here please.

Comment 50

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.2

Here is the fixed nit. Thanks

Comment 51

[Andrei Eftimie]

Comment on attachment 8422362 [details] [diff] [review]
patch_v4.2

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

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

Cosmin, please backport this patch.

Comment 52

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.2[beta7release]

previous patch applies on aurora, this patch is for beta and release.

Comment 53

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v4.2[esr24]

This is the patch for esr24.

Comment 54

[Andrei Eftimie]

Comment on attachment 8422444 [details] [diff] [review]
patch_v4.2[beta7release]

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/194432107271 (mozilla-beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/e54cac8e226c (mozilla-release)

Comment 55

[Andrei Eftimie]

Comment on attachment 8422445 [details] [diff] [review]
patch_v4.2[esr24]

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

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/6837facf8b11 (mozilla-esr24)

Comment 56

[Lawrence Mandel [:lmandel] (use needinfo)]

I shouldn't have added the 30+ flag to this bug. Removing...