RemoveDataFromDomain return promise is incomplete

RESOLVED FIXED in Firefox 55

Status

()

Firefox
General
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: mak, Assigned: milindl, Mentored)

Tracking

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [good first bug][lang=js], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

a year ago
First of all, the returned promise is result of Promise.all, that means it will return at the first rejection, rather than waiting for all of the promises. That's not nice for a cleanup method, since it should try to clear as much as possible.

Second, it is only waiting for plugin data and push notifications... the other async cleaners are ignored.

Third, no test is currently yielding on it, that is a good recipe for intermittent failures.

We should convert it to a Task.async, run the cleaners in parallel (do not serialize all the yields) and catch errors without interrupting other cleaners. basically collect all the promises, and just at the end go through them with a for loop, try/catching the results.

Finally we should update tests and code to properly yield or .catch()
(Reporter)

Updated

a year ago
Whiteboard: [good first bug][lang=js]

Comment 1

a year ago
I'd like to try working on this bug. (UofT assignment)
(Reporter)

Comment 2

a year ago
thanks, feel free to ask question (though unfortunately today I'm out).
you can start from here
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee: nobody → claudzchen

Comment 3

a year ago
Marco, could you clarify about third point of the description? I don't quite understand what yielding on it means. Also, could you run me through how I could run the tests for the function?
Flags: needinfo?(mak77)
(Reporter)

Comment 4

a year ago
(In reply to Claudia Chen from comment #3)
> Marco, could you clarify about third point of the description? I don't quite
> understand what yielding on it means. Also, could you run me through how I
> could run the tests for the function?

hi, you can search through the code tree using dxr or mxr, for example:
https://dxr.mozilla.org/mozilla-central/search?q=removeDataFromDomain&redirect=true&case=false

there are different kind of tests, but mostly we care about xpcshell-tests
https://developer.mozilla.org/docs/Mozilla/QA/Writing_xpcshell-based_unit_tests
in general these are under "unit" folders and start with "test_" prefix.
and mochitests
https://developer.mozilla.org/docs/Browser_chrome_tests
in general these start with "browser_" prefix.

in general all the tests can be run through ./mach test path_to_test

Regarding the "yielding", most of the tests are supposed to use add_task, that basically runs the code inside a Task generated by Task.jsm (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Task.jsm)
Inside a task you can wait for a promise using yield your_method_returning_promise();
Flags: needinfo?(mak77)
(Reporter)

Updated

3 months ago
Assignee: claudzchen → nobody

Comment 5

3 months ago
Hello, I would like to take this on as my first bug fix. Please let me know if I can go ahead and start working on this.
(Reporter)

Comment 6

3 months ago
Sure you can, I'll assign the bug once the first patch is attached.
(Assignee)

Comment 7

2 months ago
Hi, I would also like to work on this bug.

Regarding the first part of the post (In reply to Marco Bonardo [::mak] from comment #0)

> We should convert it to a Task.async, run the cleaners in parallel (do not
> serialize all the yields) and catch errors without interrupting other
> cleaners. basically collect all the promises, and just at the end go through
> them with a for loop, try/catching the results.

This basically means that we need to convert the entire function to a "async" Task.
Then, for each cleaner, I make a Promise and store it in some array (the promise resolves - but
with what value? - if the cleaner worked and fails otherwise). Then, I run thru the array and yeild on each element

Is that correct?
I am working on a first patch right now, I'll attach it shortly.
Thanks
(Reporter)

Comment 8

2 months ago
(In reply to milindl from comment #7)
> This basically means that we need to convert the entire function to a
> "async" Task.

Very likely.

> Then, for each cleaner, I make a Promise and store it in some array (the
> promise resolves - but
> with what value? - if the cleaner worked and fails otherwise). Then, I run
> thru the array and yeild on each element

Pretty much, you should run through all of them and try/catch for each of them, collecting in a bool whether there has been an error.
Then at the end you can check the error bool and resolve/reject based on that.

The consumers should then either yield on RemoveDataFromDomain or .catch(Cu.reportError) it they don't care about the error (for example controller.js likely doesn't care but could still report it to the console)
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
Hi, So I've submitted a patch, which needs review - I'll sum it up here since it is somewhat long-ish. There are also some points where I've had a doubt, so I have also written it here. This patch passes browser/components/contextualidentity/test/browser/browser_forgetaboutsite.js

Summary:
1. Make each cleaner into a promise which resides in a promises array.
2. At the end of this promise-making, run .then(null, (function that calls Cu.reportError and logs the error)) on each promise
3. Now we need to return the final promise value. This was slightly tricky for me - since I need to make sure all promises are either rejected or fulfilled before sending this. So for this I have converted all rejections to resolutions, and then used Promise.all to wait for all promises to resolve.

Here are the doubts - mainly stem from 2 reasons
1. Code in loops: do I make subtasks inside loops a promise or do I make the entire loop into a promise? I have gone with subtasks as promise be default, but I could not decide when the entire loop was wrapped in a try-catch block
2. There were certain cleaners not using try catch at all - do I make them an always resolving promise?
I've marked my doubts in the code as well, with a "DOUBT" in the comments.

Thanks
(Reporter)

Comment 11

2 months ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review120002

This is missing the consumers changes, see http://searchfox.org/mozilla-central/search?q=removeDataFromDomain&path=
controller.js should use .catch(Cu.reportError) while the tests should likely yield on it

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:48
(Diff revision 1)
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  this.ForgetAboutSite = {
>    removeDataFromDomain: function CRH_removeDataFromDomain(aDomain) {

I think the code wouls be much cleaner and simple if you'd define this as:

removeDataFromDomain: Task.async(function* () {
  ....
});

This converts the function to a generator, and then you can yield on promises directly, that means you can likely write the final loop as
for (let p of promises) {
  try {
    yield p;
  } catch (ex) {
    ...
  }
}

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:48
(Diff revision 1)
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  const Cu = Components.utils;
>  
>  this.ForgetAboutSite = {
>    removeDataFromDomain: function CRH_removeDataFromDomain(aDomain) {

You could have a slightly cleaner code by changing this to

removeDataFromDomain: Task.async(function* () {
  ...
});

That will allow you to yield on promises directly, so that the loop becomes
for (let p of promises) {
  try {
    yield p;
  } catch(ex) {
    ...
  }
}

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:63
(Diff revision 1)
> -    try {
> +      try {
> -      cs.clear();
> +	      cs.clear();
> +	resolve();
> -    } catch (ex) {
> +      } catch (ex) {
> -      Cu.reportError("Exception thrown while clearing the cache: " +
> +	reject("Exception thrown while clearing the cache: " +
> -        ex.toString());
> +	        ex.toString());

The toString() calls here (and in the same cases below) look pointless, afaict it will already happen by itself. Please remove them and oneline where possible.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:65
(Diff revision 1)
> +	resolve();
> -    } catch (ex) {
> +      } catch (ex) {
> -      Cu.reportError("Exception thrown while clearing the cache: " +
> +	reject("Exception thrown while clearing the cache: " +
> -        ex.toString());
> +	        ex.toString());
> -    }
> +      }
> +    }));

Sorry, I should have been more clear.
Some of these "cleaners" are synchronous, for those there's no point in wrapping them into a promise.
The only safe way to know is to check their interface on the idl file.
For example nsICacheStorageService::clear is a method returning void, it provides no way to wait for it.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:88
(Diff revision 1)
> -             getService(Ci.nsICookieManager2);
> +          getService(Ci.nsICookieManager2);
> -    let enumerator = cm.getCookiesWithOriginAttributes(JSON.stringify({}), aDomain);
> +      let enumerator = cm.getCookiesWithOriginAttributes(JSON.stringify({}), aDomain);
>      while (enumerator.hasMoreElements()) {
> +      let promise = new Promise((resolve, reject) => {
> +	try {
> -      let cookie = enumerator.getNext().QueryInterface(Ci.nsICookie);
> +	      let cookie = enumerator.getNext().QueryInterface(Ci.nsICookie);

I think you should move this enumerator.getNext() call just after the while and inside the promise closure just refer to cookie.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:94
(Diff revision 1)
> -      cm.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +	      cm.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +	  resolve();
> +	} catch (ex) {
> +	  // ignore exception and resolve anyway
> +	  // emulating model for plugin data
> +	  resolve();

since you have one promise per cookie, you could reject here, we'll handle errors later in the for loop.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:125
(Diff revision 1)
>            ph.clearSiteData(tags[i], aDomain, FLAG_CLEAR_ALL, -1, function(rv) {
>              resolve();
>            });
>          } catch (e) {
>            // Ignore errors from the plugin, but resolve the promise
>            resolve();

Again, let's reject. We'll change the way we handle the failures by making the loop ignore errors in the middle, there's no reason to fake a success.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:141
(Diff revision 1)
> -    }).then(null, Cu.reportError);
> +    }));
>  
>      // Passwords
> +    // DOUBT : The try catch is over the entire loop
> +    // I can make the entire loop a promise but it will be hard
> +    // to parallelize the tasks inside the loop

it's fine to put the entire loop into a promise, regardless this can't be parallelized since the data access is likely serialized.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:174
(Diff revision 1)
> -        }
> +          }
> +	  resolve();
> -      } catch (e) {
> +	      } catch (e) {
> -        /* Ignore entry */
> +          /* Ignore entry */
> +	  // Resolve anyway, as seen above
> +	  resolve();

please reject, as said.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:182
(Diff revision 1)
> +      promises.push(promise);
>      }
>  
>      // Offline Storages
> +    // DOUBT here:
> +    // when do I need to reject here? Or do I always resolve?

try/catch around it and reject in the catch

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:191
(Diff revision 1)
> -    // delete data from both HTTP and HTTPS sites
> +      // delete data from both HTTP and HTTPS sites
> -    let caUtils = {};
> +      let caUtils = {};
> -    let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +      let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> -                       getService(Ci.mozIJSSubScriptLoader);
> +          getService(Ci.mozIJSSubScriptLoader);
> -    scriptLoader.loadSubScript("chrome://global/content/contentAreaUtils.js",
> +      scriptLoader.loadSubScript("chrome://global/content/contentAreaUtils.js",
>                                 caUtils);

All of this looks pointless.
instead of caUtils.makeURI, this should just use NetUtil.newURI... could you please remove the loadSubscript and caUtils?

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:217
(Diff revision 1)
> +    promises.push(new Promise((resolve, reject) => {
> -    let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> +      let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> -               getService(Ci.nsIContentPrefService2);
> +          getService(Ci.nsIContentPrefService2);
> -    cps2.removeBySubdomain(aDomain, null, {
> +      cps2.removeBySubdomain(aDomain, null, {
> -      handleCompletion: () => onContentPrefsRemovalFinished(),
> +	      handleCompletion: () => onContentPrefsRemovalFinished(),
> -      handleError() {}
> +	      handleError() {}

handleCompletion should be inlined, it should invoke resolve() or reject() depending on whether handleError has been invoked first

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:224
(Diff revision 1)
> +    }));
>  
>      // Predictive network data - like cache, no way to clear this per
>      // domain, so just trash it all
> +    promises.push(new Promise((resolve, reject) => {
> -    let np = Cc["@mozilla.org/network/predictor;1"].
> +      let np = Cc["@mozilla.org/network/predictor;1"].

try/catch and reject in the catch, so we can annotate the error with who generated it.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:242
(Diff revision 1)
>  
>      // HSTS and HPKP
> +    // DOUBT: since the initial operations are also try-catched,
> +    // they should also be in a promise as done here
> +    // but this decreases asyncness since the subtasks are not run async
> +    // So is it OK to use a Task instead

I think it's ok to just wrap everything in the promise. As I said most of I/O is serialized, parallelizing won't make a big diff.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:262
(Diff revision 1)
> -            // This uri is used as a key to remove the state.
> +              // This uri is used as a key to remove the state.
> +	      let caUtils = {};
> +	      let scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
> +		  getService(Ci.mozIJSSubScriptLoader);
> +	      scriptLoader.loadSubScript("chrome://global/content/contentAreaUtils.js",
> +					 caUtils);

ditto, caUtils is pointless, just use NetUtil.newURI

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:291
(Diff revision 1)
> +    })).then((results) => {
> +      return totalErrors;
> +    }).catch((error) => {
> +      return totalErrors; // we don't care about any error here
> +    });
> +  }

Being in Task.async you should be able to write this as a simple loop where you try/catch every run.
We should Cu.reportError every error, and then reject if there is any error (that in a Task basically means you should throw if error numbers > 0)
Attachment #8844467 - Flags: review?(mak77)
(Reporter)

Comment 12

2 months ago
Sorry there has been a problem in the mozreview UI and some of the comments I had removed have been posted regardless.
I'll resolve the issues on mozreview for the ones you should ignore.
(Reporter)

Comment 13

2 months ago
Ok, the comment you can completely ignore is to avoid wrapping sync consumers in a promise. I initially thought of a different layout for this code, but then I figured it's simpler to manage centrally if everything is wrapped, so what you did is fine. I dropped the comment in mozreview, I can't drop it from here.
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
mozreview-review-reply
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review120002

I'm not totally sure about the last bit - does this mean that all of the Tests appearing in [http://searchfox.org/mozilla-central/search?q=removeDataFromDomain](http://searchfox.org/mozilla-central/search?q=removeDataFromDomain), or something else?
(Assignee)

Comment 16

2 months ago
I've made the changes mentioned in the review. 

I think it should mostly be fine, however, please take a look at Line201 handleCompletion: I've inlined it and chosen to throw the error away; I can get the error by noting it when handleError is called and then passing it to reject, but this takes a few lines extra.

But the original code does not do it, so I've chosen to throw the error away as well. Please tell me if this error should be included as well.
(Assignee)

Comment 17

2 months ago
mozreview-review-reply
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review120002

Please disregard the comment, I looked at Comment 4 on the bug thread, I am fixing the tests.
Thanks
Comment hidden (mozreview-request)
(Reporter)

Comment 19

2 months ago
(In reply to milindl from comment #15)
> I'm not totally sure about the last bit - does this mean that all of the
> Tests appearing in
> http://searchfox.org/mozilla-central/search?q=removeDataFromDomain

Yes, all the code that doesn't yield or .catch() on this call is basically ignoring an error condition, that for a test doesn't sound great.
In most of the cases it should be trivial to fix those, by adding a yield if it's already in a generator, changing functions to generators where possible, or replacing test() with add_task (that will require a bit more attention, since then you should replace the waitForFinish()/finish() system with a promise the task can yield on).
You can either do that in a separate changeset, or I'd also be fine with a follow-up separate bug.
(Reporter)

Comment 20

2 months ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review121046

::: browser/components/places/content/controller.js:258
(Diff revision 3)
>          host = queries[0].domain;
>        } else
>          host = NetUtil.newURI(this._view.selectedNode.uri).host;
> -      ForgetAboutSite.removeDataFromDomain(host);
> +      ForgetAboutSite
> +        .removeDataFromDomain(host)
> +        .catch(Cu.reportError);

In general we indent aligning dots, so it would be:
ForgetAboutSite.removeDataFromDomain(host)
               .catch...

Also (my fault) you need to use the complete Components.utils.reportError, because this is directly imported in windows where we can't be sure Ci, Cu, Cc shortcuts are available.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:1
(Diff revision 3)
> +

not sure if it's an artifact of mozreview, but looks like you added a newline on top of this file? PLease remove it if so.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:90
(Diff revision 3)
> +      let promise = new Promise((resolve, reject) => {
> +	try {
> -      cm.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +	      cm.remove(cookie.host, cookie.name, cookie.path, false, cookie.originAttributes);
> +	  resolve();
> +	} catch (ex) {
> +	  reject(ex);

Please keep using the nicer error messages like "Exception thrown while clearing..." or "Error while clearing..." everywhere, it sounds useful to know what was being cleared, but all the exceptions below are lacking it.
Also because, otherwise, it would be pointless to try/catch, since throwing inside a promise rejects it already IIRC.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:93
(Diff revision 3)
> +	  resolve();
> +	} catch (ex) {
> +	  reject(ex);
> +	}
> +      });
> +      promises.push(promise);

nit: Sometimes you do promises.push(new Promise... other times you define a let promise and push later. Maybe you could just consistently use the former.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:149
(Diff revision 3)
> -      // XXXehsan: is there a better way to do this rather than this
> +	      // XXXehsan: is there a better way to do this rather than this
> -      // hacky comparison?
> +	      // hacky comparison?
> -      if (ex.message.indexOf("User canceled Master Password entry") == -1) {
> +	      if (ex.message.indexOf("User canceled Master Password entry") == -1) {
> -        throw ex;
> +	  reject(ex);
> -      }
> +	      }
> -    }
> +      }

Where is this resolved?

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:199
(Diff revision 3)
>  
>      // Content Preferences
> +    promises.push(new Promise((resolve, reject) => {
> -    let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> +      let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> -               getService(Ci.nsIContentPrefService2);
> +          getService(Ci.nsIContentPrefService2);
> +      // subject to tests

what does this mean?

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:202
(Diff revision 3)
> -    let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> +      let cps2 = Cc["@mozilla.org/content-pref/service;1"].
> -               getService(Ci.nsIContentPrefService2);
> +          getService(Ci.nsIContentPrefService2);
> +      // subject to tests
> -    cps2.removeBySubdomain(aDomain, null, {
> +      cps2.removeBySubdomain(aDomain, null, {
> -      handleCompletion: () => onContentPrefsRemovalFinished(),
> +	handleCompletion: (reason) => {
> +	  // Everybody else (including extensions)

While here please fix this comment as:
// Notify other consumers, including extensions.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:204
(Diff revision 3)
> +      // subject to tests
> -    cps2.removeBySubdomain(aDomain, null, {
> +      cps2.removeBySubdomain(aDomain, null, {
> -      handleCompletion: () => onContentPrefsRemovalFinished(),
> +	handleCompletion: (reason) => {
> +	  // Everybody else (including extensions)
> +	  Services.obs.notifyObservers(null, "browser:purge-domain-data", aDomain);
> +	  if (reason === cps2.REASON_ERROR) reject(); // The error is not returned

I don't think this works, these are the possible values for reason:
http://searchfox.org/mozilla-central/rev/7cb75d87753de9103253e34bc85592e26378f506/dom/interfaces/base/nsIContentPrefService2.idl#395

You should use Ci.nsIContentPrefCallback2.COMPLETE_ERROR
And reject something useful, like new Error("Error while clearing content preferences")

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:205
(Diff revision 3)
> -    cps2.removeBySubdomain(aDomain, null, {
> +      cps2.removeBySubdomain(aDomain, null, {
> -      handleCompletion: () => onContentPrefsRemovalFinished(),
> +	handleCompletion: (reason) => {
> +	  // Everybody else (including extensions)
> +	  Services.obs.notifyObservers(null, "browser:purge-domain-data", aDomain);
> +	  if (reason === cps2.REASON_ERROR) reject(); // The error is not returned
> +	  else resolve();

please indent as:
if (something)
  do_something
else 
  do_something_else

if any of the bodies is longer than 1 line, then bracing is mandatory.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:269
(Diff revision 3)
> +      } catch (ex) {
> +	Cu.reportError(ex);
> +	totalErrors.push(ex);
> +      }
> +    }
> +    if (totalErrors.length !== 0) throw new Error(totalErrors);

please always indent bodies
since we already printed all the errors to the console, here we could just cache an ErrorCount and do
throw new Error(`There were a total of ${ErrorCount} errors during removal`).
Attachment #8844467 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

2 months ago
(In reply to milindl from comment #22)
> Comment on attachment 8844467 [details]
> Bug 1247201 - Run cleaners async to clear as much as possible,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/117906/diff/4-5/

This request will, I think, be sufficient. I've tried to fix as many stylistic errors as I could find, since they are the main issue in the previous requests. Apart from the changes requested, I have fixed some that I found myself.

I also reverted the changes made to the Unit Test, since that needed to be worked upon more, so I'll be submitting a separate changeset.
Thanks
Comment hidden (mozreview-request)
(Reporter)

Comment 25

a month ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review122026

It's mostly good, I only found one missing resolve.
Waiting for the tests part too.

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:146
(Diff revision 6)
> -    } catch (ex) {
> +      } catch (ex) {
> -      // XXXehsan: is there a better way to do this rather than this
> +        // XXXehsan: is there a better way to do this rather than this
> -      // hacky comparison?
> +        // hacky comparison?
> -      if (ex.message.indexOf("User canceled Master Password entry") == -1) {
> +        if (ex.message.indexOf("User canceled Master Password entry") == -1) {
> -        throw ex;
> +          reject("Exception occured in clearing passwords :" + ex);
> -      }
> +        }

should resolve() in the else case, or use "promises.push(Task.spawn" and throw in the if case.
Regardless, it should be resolved somehow when we ignore the exception.
Attachment #8844467 - Flags: review?(mak77)
(Reporter)

Comment 26

a month ago
Please mark the issues that you solved as FIXED on mozreview, otherwise we won't be able to use autoland when it's the time.
Comment hidden (mozreview-request)
(Assignee)

Comment 28

a month ago
(In reply to milindl from comment #27)
> Comment on attachment 8844467 [details]
> Bug 1247201 - Run cleaners async to clear as much as possible,
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/117906/diff/6-7/

This will adress the issue. I've also marked the relavant things "Fixed".
I'll get on to the tests. Thanks for the help :)
(Assignee)

Updated

a month ago
Depends on: 1347422
(Reporter)

Comment 29

a month ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review122442

Sorry there is still a typo, once that is fixed I can push to the Try server to check for tests.

::: browser/components/places/content/controller.js:257
(Diff revision 7)
>          var queries = this._view.selectedNode.getQueries();
>          host = queries[0].domain;
>        } else
>          host = NetUtil.newURI(this._view.selectedNode.uri).host;
> -      ForgetAboutSite.removeDataFromDomain(host);
> +      ForgetAboutSite.removeDataFromDomain(host)
> +                     .catch(Component.utils.reportError);

Should be Components (plural), could be I made a typo
Attachment #8844467 - Flags: review?(mak77)
Comment hidden (mozreview-request)
(Reporter)

Comment 31

a month ago
Note: I didn't look at the tests patch yet.
There are various test failures, indeed this can't land without the fixes in bug 1347422.
I'd actually suggest to close bug 1347422 and rather move that changeset to this bug, as a second changeset in mozreview. it shouldn't be too hard, just matter of a rebase and then changing the commit message on that, and pushing again to mozreview. That would help running both on automation (try) and regardless these 2 changesets cannot land separately.

The failures I see are in:
toolkit/forgetaboutsite/test/browser/browser_clearplugindata.js
browser/components/migration/tests/unit/test_Chrome_cookies.js

There are various exceptions while clearing plugins and permissions. Note that these could also be expected, we must figure out why they happen and then eventually we'll "ignore" those failures.
The clearSiteData exceptions should likely be verified through a cpp debugger, to check at which point clearsitedata throws. Likely the same for the permission manager.
I can eventually help with that.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 34

a month ago
Marco:

I've done the commit closing and resubmitting a patch.

As of now, on running the tests locally, I get a failure in 
browser/components/sessionstore/test/browser_464199.js | Found an unexpected browser window at the end of test run

I cannot see why this could be, since there is a call to close windows on which I yield on.

The Chrome cookie test runs fine on my PC (it is 'skipped'), so apparently it passes by default. Unsure of this.

The clearplugin test does fail, which is to be expected, since I have not made any changes to the test. I'd be happy to verify it if you can guide me through the process. I also tried experimenting with it a bit. Changing the null on line 84 to ["localhost"] makes the test pass, but I'm sure that defeats the point of the test somehow

A few other doubts are marked in the file itself.

Thanks!
(Reporter)

Comment 35

a month ago
(In reply to milindl from comment #34)
> The Chrome cookie test runs fine on my PC (it is 'skipped'), so apparently
> it passes by default. Unsure of this.

It only runs on Windows, I think.

I'll try to have a look at the tests along the day. I have a decent queue of requests to handle atm, and on monday I'm traveling, so it may take a little bit of time, but I hope to have answers sooner than later.
(Reporter)

Comment 36

a month ago
Looks like mozreview has messed up things, There should be 2 changesets and 2 review requests, I see 3. Maybe you have pushed another head to mozreview, so now it sees both of them, one with the old changeset and one with both.

The other strange thing is that the first changeset has 2 commit messages and as such it has 2 different MozReview-Commit-ID. That's likely also why it's seen twice. In every changeset there should only be one commit id afaik.

In the worst case we'll move to a usual diff if we can't fix the problem.
(Reporter)

Comment 37

a month ago
Fwiw, I suggest using the hg evolve extension, and hg histedit may also be useful.
(Reporter)

Comment 38

a month ago
To better explain, you were in this situation:
 _o changeset-2 (the other bug, in its own head)
|_o changeset-1 (this bug, in its own head)
|_o mozilla-central

I would have done hg rebase -s changeset-2 -d changeset-1
  o changeset-2 (the other bug, in same head as changeset-1)
 _o changeset-1 (this bug in its own head)
|_mozilla-central

Then hg update head-2 && hg commit --amend (to edit the commit message and fix the bug #, alternatively histedit can do the same using "mess").
At that point pushing to mozreview should have just added the second changeset without touching the first one.

To edit changeset-1 it's enough to hg update changeset-1, make the modifications, hg amend and hg evolve --all (to rebase changeset-2 in place)

hg wip can help you keeping the heads situation under control.
Not a critic, just trying to help. There are multiple possible workflows and this is just one of them.
(Assignee)

Comment 39

a month ago
Thank you for explaining, I was pretty confused myself by the 3 requests I sent.

I actually got a very frustrating "abort: authentication error" message for quite a while, so I cloned the entire repo again. After that, I have been following the "multiple heads" workflow (with hg wip, but histedit only occasionally. I didn't use evolve.)

I think the re-clone possibly led to a different MozReview ID (just taking a stab at it; it's unlikely anyone else has gone through the exact same steps).

Further, my 1st and 2nd review requests are the same (they both have 2 commit messages); except for the fact that the first one has 8 intermediate diffs.

I'm sorry for the issue, I'll try to fix this!
(Reporter)

Comment 40

a month ago
mozreview-review
Comment on attachment 8848432 [details]
Bug 1247201 - yield on tests using removeDataFromDomain,

https://reviewboard.mozilla.org/r/121342/#review123438

I'm still investigating the other 2 failures.

::: browser/components/sessionstore/test/browser_464199.js:81
(Diff revision 1)
> -
> -        // clean up
> +    // clean up
> -        gPrefService.clearUserPref("browser.sessionstore.max_tabs_undo");
> +    gPrefService.clearUserPref("browser.sessionstore.max_tabs_undo");
> -        BrowserTestUtils.closeWindow(newWin).then(finish);
> +    yield BrowserTestUtils.closeWindow(newWin);
> +    // DOUBT: do we need to still call this? finish();
>      });

here is the problem:
first, no, you don't need to call finish() when you use add_task, it's the task itself that defines when the test finishes by yielding.
The problem here is that your task is finishing after the call to removeDataFromDomain because you are not waiting for waitForClearHistory. It's not enough to yield inside it since you are yielding in a callback (it won't work too). You test is finishing prematurely because you're not waiting all of it.

what you need to do is convert waitForClearHistory to promiseclearHistory, and make it return a promise you can yield on in the main task.
then move all the callback code to happen after that yield.
Attachment #8848432 - Flags: review?(mak77)
(Reporter)

Comment 41

a month ago
Looks like some cleaners don't like parallel execution and that's why some of tests are failing.
Additionally, the plugin data clearer throws in a lot of cases, since we only support cleaning from flash or loaded plugins (I debugged it in nsPluginHost.cpp). The API is not great since it's impossible to distinguish errors from wanted bailouts. Some of those should probably have been simple returns rather than errors. Let's go back to ignoring any failures for plugins and always resolve.
We may have to do the same for permissions too, where the old code had /* Ignore entry */ :(

I'd say in all the cases where there's a loop and you are adding one promise per looping, let's go back to a single promise for the whole cleaner.
Also, in the cases where the code is synchronous and we end up with:
push(new Promise(resolve, reject) {
  try {
    ...
    resolve();
  } catch (ex) {
    reject(...);
  }
});

let's instead go with:
push(Task.spawn(function*() {
  ...
}).catch(ex => {
  Cu.reportError(...);
  throw ex;
});

Sorry for the double work, at this point let's concentrate on the main scope of the bug, that is to make sure each cleaner doesn't throw us out of removeDataFromDomain and that consumers wait on it.
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8848431 - Attachment is obsolete: true
Attachment #8848431 - Flags: review?(mak77)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a month ago
Attachment #8848985 - Flags: review?(mak77)
Comment hidden (mozreview-request)
(Assignee)

Comment 48

a month ago
Marco: 

I've fixed most of the MozReview issues. (There's one patch, "3 days ago" in which there's no reviewer, and that's fine, because the changeset contained in that is not required. Wasn't sure how to get rid of the extra MozReview page since that uses a different bz:// id).

Also, note that the patch I've submitted is not yet done (the tests one has some progress however ). I'll submit another patch in half a day; but before that on the conversion to Task.spawn - if we do that, won't we risk losing some cleaners? 

If we have Task.spawn((generator)).catch, doesn't the generator throw on encountering the first error? This seems fine in Passwords, but in Plugins for instance, this does not seem correct, since we may not run some plugin cleaner at all (given the fact that we need to ignore a lot of plugin exceptions).
(Reporter)

Comment 49

a month ago
(In reply to milindl from comment #48)
> If we have Task.spawn((generator)).catch, doesn't the generator throw on
> encountering the first error? This seems fine in Passwords, but in Plugins
> for instance, this does not seem correct, since we may not run some plugin
> cleaner at all (given the fact that we need to ignore a lot of plugin
> exceptions).

We should restore previous behavior where internally in the loop we try/catch and ignore exceptions.
For those 2 cases we can't do better atm.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

a month ago
Marco:

I've mostly done the changes. There are however a few things I should point out:
All test except clearplugindata pass; the chrome one I can't check. To deal with this, I tried doing the following - replacing my code completely with the old code; replacing the plugin-specific code with synchronous code (no promise). None of this worked. Finally, I found that if I replace

  83 yield ForgetAboutSite.removeDataFromDomain("localhost");
  84 ok(!stored(null), "All data cleared");

with

  83 yield ForgetAboutSite.removeDataFromDomain("localhost");
  84 stored(null);
  85 ok(!stored(null), "All data cleared");

It works! Essentially, two subsequent calls of stored(null) give a different result on my PC, at least. I find this intensely disturbing. 

I used histedit to take the changes to my previous commit. I'm now pretty comfortable about the many head workflow :) 

Thanks!
(Reporter)

Updated

a month ago
Attachment #8848432 - Attachment is obsolete: true
(Reporter)

Comment 53

a month ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review123964

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:257
(Diff revision 11)
> -    return Promise.all(promises);
> +    let ErrorCount = 0;
> +    for (let promise in promises) {
> +      try {
> +        yield promise;
> +      } catch (ex) {
> +        Cu.reportError(ex);

if in all the catches we'd just throw new Error("Exception occured while...: " + ex), we could remove all the reportErrors above and just keep this one.
Attachment #8844467 - Flags: review?(mak77)
(Reporter)

Comment 54

a month ago
also, I fear I won't be able to investigate the clearpluginsdata problem until I'm back home on Thursday
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 57

a month ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review125440

I think I found the problem, the for...in is wrong.
This means we must run again all the tests to ensure they pass, since now we will properly wait and catch exceptions. We can do a Try run once this is updated.
Sorry if it took more days than expected, I just returned home :)

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:105
(Diff revision 12)
>      for (let i = 0; i < tags.length; i++) {
> -      let promise = new Promise(resolve => {
> +      promises.push(new Promise(resolve => {
>          try {
>            ph.clearSiteData(tags[i], aDomain, FLAG_CLEAR_ALL, -1, function(rv) {
>              resolve();
>            });

could probably be just ", resolve);" instead of ", function (rv) { resolve() });"

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:155
(Diff revision 12)
> -          pm.removePermission(perm);
> +            pm.removePermission(perm);
> -        }
> +          }
> -      } catch (e) {
> -        /* Ignore entry */
> +          resolve();
> +        } catch (ex) {
> +          // Ignore entry
> +          resolve();

Looks like we should invoke resolve() in a finally clause. (it's ok to keep just the comment in the catch body)

::: toolkit/forgetaboutsite/ForgetAboutSite.jsm:242
(Diff revision 12)
> -      Cu.reportError("Exception thrown while clearing HSTS/HPKP: " +
> -                     e.toString());
> +      throw new Error("Exception thrown while clearing HSTS/HPKP: " + ex);
> +    }));
> -    }
>  
> -    return Promise.all(promises);
> +    let ErrorCount = 0;
> +    for (let promise in promises) {

This is subtly wrong, it should be a for...of, not a for...in.
This means that basically the code is not properly waiting on the promises, that may also explain the plugin data problem, since we check too early.
Attachment #8844467 - Flags: review?(mak77)
(Reporter)

Comment 58

a month ago
mozreview-review
Comment on attachment 8848985 [details]
Bug 1247201 - yield on tests using removeDataFromDomain,

https://reviewboard.mozilla.org/r/121838/#review125446

these changes look good, provided the tests will pass once you fix the bug found in the other commit.

::: toolkit/forgetaboutsite/test/browser/browser_clearplugindata.js:51
(Diff revision 5)
>        pluginTag = tags[i];
>      }
>    }
>    if (!pluginTag) {
>      ok(false, "Test Plug-in not available, can't run test");
>      finish();

There is a small nit problem here, where we invoke finish() insize add_task. Not your fault but may be nice to fix it while you're at it.

Basically just replace this with a return; merge the 2 tasks into one and remove the "setup" name for the generator (since it will now run all the test).
Attachment #8848985 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 61

a month ago
mozreview-review
Comment on attachment 8844467 [details]
Bug 1247201 - Run cleaners async to clear as much as possible

https://reviewboard.mozilla.org/r/117906/#review126700
Attachment #8844467 - Flags: review?(mak77) → review+
(Reporter)

Comment 62

a month ago
Thank you! it looks good now, landing!
(Reporter)

Updated

a month ago
Assignee: nobody → i.milind.luthra
Status: NEW → ASSIGNED

Comment 63

a month ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/0acbd98f996d
Run cleaners async to clear as much as possible r=mak
https://hg.mozilla.org/integration/autoland/rev/860cafd805c8
yield on tests using removeDataFromDomain,r=mak
(Assignee)

Comment 64

a month ago
A few tests show up as busted or failed when I open it in treeherder, however. They _seem_ to be unrelated to this, does this happen usually?

Thanks for the help, Marco!
(Reporter)

Comment 65

a month ago
(In reply to milindl from comment #64)
> A few tests show up as busted or failed when I open it in treeherder,
> however. They _seem_ to be unrelated to this, does this happen usually?

yes, we do have intermittent failures on some tests. With so many tests and async behavior it's something very hard to avoid.
(Assignee)

Comment 66

a month ago
Should we(Can we?) fix this, or does this stay as it is?
(Reporter)

Comment 67

a month ago
no, there's nothing to fix here, there's a separate bug filed for the intermittent failure, we have many. Any work should eventually happen there, but often it's not trivial to fix those issues because they are hard to reproduce on the developer machine.
You're done here, and you can pick other mentored bugs (https://www.joshmatthews.net/bugsahoy/?), or attack your own favorite component around. Most components have bugs with a priority set from P1 to P5, the ones that matter most are usually between P1 and P3 (included), where P1 is usually bugs for which a team has already found resources. So P2s or P3s are both plausible targets.

If you're looking for a more complex bug, among my mentored bugs I may suggest:
Bug 1095426 (refactoring), Bug 1095427 (refactoring), Bug 1036440 (almost a rewrite), Bug 1107364 (perf fix), bug 1249263 (writing a new API).
(Assignee)

Comment 68

a month ago
I'm currently working on a bug in sessionstore(I'm applying for the sessionstore GSoC). Since I am looking for a more complex bugs, I'll go through the linked bugs and try understanding and submitting a first patch as soon as possible for me.

Thank you! :)
(Reporter)

Comment 69

a month ago
That's awesome, keep up the good work!

Comment 70

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0acbd98f996d
https://hg.mozilla.org/mozilla-central/rev/860cafd805c8
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.