I think what is happening is: * PlacesCategoryStarter.js gets idle-daily, calls PDBU_maintenanceOnIdle * One of the scheduled tasks is checkIntegrity * PDBU_checkIntegrity issues an asyncStatement (call it stmtX) * We call asyncClose * stmtX calls its completition callback * the completition callback calls PDBU__executeTasks * PDBU__executeTasks calls PDBU_checkCoherence I added some logging and pushed to try to be sure that is what is happening. I can see two ways of fixing this: * Have stmtX's handleCompletition check if asyncClose was called and if so don't call PDBU__executeTasks. * add a completition callback to PlacesDBUtils.maintenanceOnIdle and use that to spin the loop when we get PlacesUtils.TOPIC_SHUTDOWN. I will try to implement the first option.

Yes, that is what is happening: https://tbpl.mozilla.org/php/getParsedLog.php?id=8670368&tree=Try&full=1#error1

Comment on attachment 589923 [details] [diff] [review] PDBU_maintenanceOnIdle can try use the places database after it is closed Review of attachment 589923 [details] [diff] [review]: ----------------------------------------------------------------- I see, if it happens in tests it's likely because test harness is permanently idle, indeed I disabled idle notifications in most of the harnesses to avoid useless maintenance tasks hitting them. In real life it's unlikely the user may close the browser while he's idle, even if not impossible. ::: toolkit/components/places/PlacesCategoriesStarter.js @@ +94,5 @@ > switch (aTopic) { > case PlacesUtils.TOPIC_SHUTDOWN: > Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN); > Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY); > + PlacesDBUtils.isShuttingDown = true; So, two things. First you should check if PlacesDBUtils is a getter, and use it only if it's not, otherwise we are going to import the module just to tell it we are shutting down. Second I'd prefer if you'd add a .shutdown() method to PlacesDBUtils rather than setting a property, then you may do what you want inside it :) This will cover eventual future needs. ::: toolkit/components/places/PlacesDBUtils.jsm @@ +265,5 @@ > } > + if (PlacesDBUtils.isShuttingDown) { > + tasks.log("- We are shutting down. Will not schedule the tasks."); > + return; > + } I prefer if you handle this inside _executeTasks() than inside a single one, that internal method is globally used to schedule the next task so should be safer to future changes. There just log as you do here and clear() the aTasks array. At that point it should just not find a next task and notify the end.\ Makes sense?

> I see, if it happens in tests it's likely because test harness is > permanently idle, indeed I disabled idle notifications in most of the > harnesses to avoid useless maintenance tasks hitting them. > In real life it's unlikely the user may close the browser while he's idle, > even if not impossible. On the bots this was happening during profile creation run of firefox... > > + PlacesDBUtils.isShuttingDown = true; > > So, two things. > First you should check if PlacesDBUtils is a getter, and use it only if it's > not, otherwise we are going to import the module just to tell it we are > shutting down. > Second I'd prefer if you'd add a .shutdown() method to PlacesDBUtils rather > than setting a property, then you may do what you want inside it :) This > will cover eventual future needs. What is the JS incantation for "is this a getter"? The Object.getOwnPropertyDescriptor thing? > I prefer if you handle this inside _executeTasks() than inside a single one, > that internal method is globally used to schedule the next task so should be > safer to future changes. There just log as you do here and clear() the > aTasks array. At that point it should just not find a next task and notify > the end.\ > Makes sense? Maybe. I will upload a new patch.

(In reply to Rafael Ávila de Espíndola (:espindola) from comment #4) > What is the JS incantation for "is this a getter"? The > Object.getOwnPropertyDescriptor thing? Yes that ugly verbose thing may work

Assuming it works, would a patch like this but without the dump calls be OK? https://tbpl.mozilla.org/?tree=Try&rev=4b4e45fd53b0

Comment on attachment 590202 [details] [diff] [review] PDBU_maintenanceOnIdle can try use the places database after it is closed Review of attachment 590202 [details] [diff] [review]: ----------------------------------------------------------------- yes, provided it works fine and with the following changes: ::: toolkit/components/places/PlacesCategoriesStarter.js @@ +97,3 @@ > Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN); > Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY); > + let globalDict = Cu.getGlobalForObject(PlacesCategoriesStarter); Dict may be misleading (since we have Dict.jsm that is another thing), globalObj will be fine @@ +97,4 @@ > Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN); > Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY); > + let globalDict = Cu.getGlobalForObject(PlacesCategoriesStarter); > + let x = Object.getOwnPropertyDescriptor(globalDict, "PlacesDBUtils"); s/x/descriptor/ ::: toolkit/components/places/PlacesDBUtils.jsm @@ +106,5 @@ > Services.obs.notifyObservers(null, FINISHED_MAINTENANCE_TOPIC, null); > } > }, > > + isShuttingDown : false, prefix with _ since it's internal @@ +107,5 @@ > } > }, > > + isShuttingDown : false, > + shutdown : function PDBU_shutdown() { no space after the label per module convention, so shutdown: function...

Now with the missing +const Cu = Components.utils; https://tbpl.mozilla.org/?tree=Try&rev=a52adc40d672

this patch does exactly what you asked. Is it OK? https://tbpl.mozilla.org/?tree=Try&rev=ca94b729166e

> yes, provided it works fine and with the following changes: I have no idea when the getGlobalForObject magic works or not, but that seems to be the best I can get r+, so... I will check in https://tbpl.mozilla.org/?tree=Try&rev=57c755c1902c once the try finishes.

Comment on attachment 590249 [details] [diff] [review] PDBU_maintenanceOnIdle can try use the places database after it is closed Review of attachment 590249 [details] [diff] [review]: ----------------------------------------------------------------- minus for the typo, though yeah, it will work once that is fixed. ::: toolkit/components/places/PlacesCategoriesStarter.js @@ +95,5 @@ > switch (aTopic) { > case PlacesUtils.TOPIC_SHUTDOWN: > Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN); > Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY); > + let globalDict = Cu.getGlobalForObject(tegoriesStarter.prototype); make it globalObj also, there is a typo here (tegoriesStarter @@ +96,5 @@ > case PlacesUtils.TOPIC_SHUTDOWN: > Services.obs.removeObserver(this, PlacesUtils.TOPIC_SHUTDOWN); > Services.obs.removeObserver(this, TOPIC_GATHER_TELEMETRY); > + let globalDict = Cu.getGlobalForObject(tegoriesStarter.prototype); > + let x = Object.getOwnPropertyDescriptor(globalDict, "PlacesDBUtils"); s/x/descriptor/ ::: toolkit/components/places/PlacesDBUtils.jsm @@ +106,5 @@ > Services.obs.notifyObservers(null, FINISHED_MAINTENANCE_TOPIC, null); > } > }, > > + isShuttingDown : false, _isShuttingDown since it's for internal use please, and remove the whitespace before the colon

Looks like try lost the previous run :-( I rebased (same patch otherwise) and pushed to https://tbpl.mozilla.org/?tree=Try&rev=e6181374c550

Comment on attachment 590910 [details] [diff] [review] PDBU_maintenanceOnIdle can try use the places database after it is closed Review of attachment 590910 [details] [diff] [review]: ----------------------------------------------------------------- hm bugzilla is kidding me, I had already reviewed this hours ago... oh well... Sorry for late.