Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ff9ac8a09604

Unfortunately, the change seems to be incompatible with clean-on-shutdown, as we attempt to access the database after it has been closed. Solution 1, discussed over IRC: <mak>ok, so maybe Sqlite.jsm should block earlier <mak>profile-change-teardown? <mak>I wonder if making Sqlite.jsm block earlier would also solve the warnings (due to not finalizing statements)

Solution 2: - Introduce a barrier on the db held by Database.cpp (say `DatabaseShutdown`); - `DatabaseShutdown` is exposed to both native code and JS; - `DatabaseShutdown` is now in charge of closing the db (used to be `Database`); - `DatabaseShutdown` blocks profile-before-shutdown until the db is closed; - `Database` blocks `DatabaseShutdown` until `Database::shutdown()`; - `History.jsm` also blocks `DatabaseShutdown` while it uses the database; - to be on the safe side, Sanitizer.prototype.sanitize also blocks `DatabaseShutdown` during its execution. Since shutdown sanitization is triggered during places-shutdown, `Sanitizer.prototype.sanitize` will be started before profile-before-shutdown, so before DatabaseShutdown has started waiting, so I think we should be ok.

/r/4971 - Bug 1089695 - Port sanitize dialog to History.jsm;r=mak /r/4973 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak /r/4975 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak Pull down these commits: hg pull review -r 6c4740a467b5bf280d82f617fccdb3332276ac3c

Small update, this is my first priority after I'm done with bug 1125115. That is blocking a lot of other stuff so I must complete it. Hope to push it today and then move to this request.

Thanks. Note that I have context-switched to something entirely unrelated in the meantime, but I'll try and be responsive.

https://reviewboard.mozilla.org/r/4975/#review4899 If you could setup your editor so it won't fix any trailing whitespace around, it would be awesome. The approach looks ok, there's some comments to address though. ::: toolkit/components/places/Database.h (Diff revision 1) > + GetConnectionShutdown(); nit: in header files usually we don't put name on a new line ::: toolkit/components/places/Database.cpp (Diff revision 1) > +#define TOPIC_SIMULATE_PROFILE_BEFORE_CHANGE "simulate-profile-before-change" so, I think it's worth to put the word "test" into the topic. Something like "places-tests-profile-before-change" But actually, does this really need to be profile-before-change notification or can we name it with a more generic places-tests-simulate-shutdown? ::: toolkit/components/places/Database.cpp (Diff revision 1) > +class DatabaseShutdown MOZ_FINAL: use final instead of MOZ_FINAL and add space before the colon. nit: we usually indent as class something final : public Ia , public Ib ::: toolkit/components/places/Database.cpp (Diff revision 1) > + DatabaseShutdown(Database* aDatabase); add explicit ::: toolkit/components/places/Database.cpp (Diff revision 1) > + GetClient(); nit: no newline in header files please ::: toolkit/components/places/Database.cpp (Diff revision 1) > + nsCOMPtr<nsIAsyncShutdownClient> mParentClient; I'm assuming barrier and client are thread-safe, right? ::: toolkit/components/places/Database.cpp (Diff revision 1) > + NOTIFIED_NO_SERVICE_PLACES_WILL_CLOSE_CONNECTION, could you please also document CALLED_WAIT, and what's NOTIFIED_NO_SERVICE_PLACES_WILL_CLOSE_CONNECTION? ::: toolkit/components/places/Database.cpp (Diff revision 1) > + RECEIVED_COMPLETE, RECEIVED_STORAGE_COMPLETE? There are too many completion callbacks here, let's try to distinguish naming. ::: toolkit/components/places/Database.cpp (Diff revision 1) > + NOTIFIED_NO_SERVICE_PLACES_CONNECTION_CLOSED I'd really like to understand the difference between "notified observer" and "notified no service" here too... ::: toolkit/components/places/Database.cpp (Diff revision 1) > +uint64_t DatabaseShutdown::sCounter = 0; does this really need to be a so large int? If it's just for tests, I'd assume a uint16_t would suffice. ::: toolkit/components/places/Database.cpp (Diff revision 1) > + : mParentClient(nullptr) Since it's a COMPtr, you don't have to initialize it. ::: toolkit/components/places/Database.cpp (Diff revision 1) > + DebugOnly<nsresult> rv = service->MakeBarrier( Either check service, or NS_ENSURE_TRUE it. I don't think we should crash in face of the user, even if we ignore an error condition. ::: toolkit/components/places/Database.cpp (Diff revision 1) > + return client.forget(); client ? client.forget: nullptr; ::: toolkit/components/places/Database.cpp (Diff revision 1) > + I think you want to NS_ENSURE_SUCCESS (or similar) here ::: toolkit/components/places/Database.cpp (Diff revision 1) > + ditto ::: toolkit/components/places/Database.cpp (Diff revision 1) > + rv = mBarrier->GetState(getter_AddRefs(barrierState)); null check mBarrier ::: toolkit/components/places/Database.cpp (Diff revision 1) > + NS_ENSURE_SUCCESS ::: toolkit/components/places/Database.cpp (Diff revision 1) > + mParentClient = aParentClient; should we MOZ_ASSERT aParentClient? ::: toolkit/components/places/Database.cpp (Diff revision 1) > + DebugOnly<nsresult> rv = mBarrier->Wait(this); null check mBarrier ::: toolkit/components/places/Database.cpp (Diff revision 1) > + mState = NOTIFIED_NO_SERVICE_PLACES_WILL_CLOSE_CONNECTION; Is this really useful? The fact observer service may not be available is very rare ::: toolkit/components/places/Database.h (Diff revision 1) > + nsRefPtr<class DatabaseShutdown> mConnectionShutdown; Is this a typo? I don't think you need to specify "class" here. Also, if I read this correctly, Database keeps alive DatabaseShutdown, that keeps alive Database... Who is breaking this cycle? Might be worth to comment here about the cycle handling (basically, when this will be nullified) ::: toolkit/components/places/Database.cpp (Diff revision 1) > + mState = NOTIFIED_NO_SERVICE_PLACES_CONNECTION_CLOSED; ditto I'd probably just invert the if ::: toolkit/components/places/Database.cpp (Diff revision 1) > + profileBeforeChange->AddBlocker( null check profileBeforeChange ::: toolkit/components/places/Database.cpp (Diff revision 1) > + nsCOMPtr<nsIAsyncShutdownBlocker> blocker = mConnectionShutdown.get(); We do we need to create a new comptr? shouldn't AddBlocker do that? Is this a try to static_cast with a hammer? :) ::: toolkit/components/places/Database.cpp (Diff revision 1) > + mConnectionShutdown = nullptr; I think here you rather want to mConnectionShutdown.forget(closeListener) or similar, rather than swap. ::: toolkit/components/places/Database.cpp (Diff revision 1) > + return mConnectionShutdown->GetClient(); MOZ_ASSERT(mConnectionShutdown) ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > + promiseRWDBConnection: () => gAsyncRWDBConnPromised, This is likely bitrotted now, I merged the connections from Bookmarks and others into a single promise here. please merge with the new code ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 1) > - let newOrder = []; > + let newOrder = []; nit: the patch would be much smaller if your editor wouldn't try to fix every trailing space around :) ::: toolkit/components/places/nsNavHistory.cpp (Diff revision 1) > + client.forget(_shutdownClient); <p>should null check client.</p> ::: toolkit/components/places/tests/head_common.js (Diff revision 1) > +function promiseClearHistory() { This should already exist in PlacesTestUtils.jsm... ::: toolkit/components/places/tests/head_common.js (Diff revision 1) > +let startupPlaces = Task.async(function*() { This is unused, and I'm not sure what's its scope, the connections are lazy, as well as history. Why do we need to initialize everything like this? Browser won't do, so we would be testing a non real env. ::: toolkit/components/places/tests/head_common.js (Diff revision 1) > +// do_print("shutdownPlaces: sent simulate-profile-before-change"); debug spew ::: toolkit/components/places/tests/history/test_remove.js (Diff revision 1) > - * License, v. 2.0. If a copy of the MPL was not distributed with this > + * http://creativecommons.org/publicdomain/zero/1.0/ */ nit: if you change the license, than just remove it, there's no more the need to put a license into test files. ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revision 1) > + * http://creativecommons.org/publicdomain/zero/1.0/ */ nit: can remove the license ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revision 1) > + PlacesUtils.bookmarks.insertBookmark( please use the new bookmarks api yield PlacesUtils.bookmarks.insert({ url: uri, parentGuid: PlacesUtils.bookmarks.unfiledGuid, title: "test bookmark" }); ::: toolkit/components/places/tests/history/test_removeVisitsByFilter.js (Diff revision 1) > + yield promiseAddVisits(visits); yield PlacesTestUtils.addVisits... ::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js (Diff revision 1) > - s.sanitize(); > + info(s.sanitize); debug spew? ::: toolkit/components/thumbnails/test/browser_thumbnails_storage.js (Diff revision 1) > - executeSoon(aCallback); > + executeSoon(aCallback); I guess w don't need to executeSoon anymore since the promise already resolves to the next tick

https://reviewboard.mozilla.org/r/4973/#review4913 ::: toolkit/components/places/History.jsm (Diff revision 1) > - Sqlite.wrapStorageConnection({ connection: PlacesUtils.history.DBConnection } ) > + let db = yield PlacesUtils.promiseRWDBConnection(); will have to change this, due to new code in PlacesUtils. Also, bug 1091851 could be a good reason to unify all the paths... We need the wrappers to close before the main connection, in any case. ::: toolkit/components/places/History.jsm (Diff revision 1) > + } I guess we might want to check if beginDate <= endDate? My only fear here is that if the opposite is true, since they are inclusive, we'll end up removing only beginDate and endDate, while we should instead throw (it's clearly a caller mistake) Having a test for this would be nice.

https://reviewboard.mozilla.org/r/4971/#review4915 ::: browser/base/content/sanitize.js (Diff revision 1) > "resource://gre/modules/Promise.jsm"); could you please remove Promise.jsm from here? ::: browser/base/content/sanitize.js (Diff revision 1) > + this._itemsToClear = {}; You actually want a Map here? ::: browser/base/content/sanitize.js (Diff revision 1) > - if (aCanClear) > + TelemetryStopwatch.start("FX_SANITIZE_TOTAL", refObj); I think the idea here was to measure the total time taken to clear ALL items, but to me looks like you changed it to the time taken to clear each item? ::: browser/base/content/sanitize.js (Diff revision 1) > - return deferred.promise; > + this._itemsToClear = {}; if you clear your forensic map, you can't use if after the call to investigate anything... is this really needed? ::: browser/base/content/sanitize.js (Diff revision 1) > - } catch (e) { } > + } catch (e) { } I wonder why we don't even log these errors to the console... ::: browser/base/content/sanitize.js (Diff revision 1) > + PlacesUtils.DBConnectionShutdown.removeBlocker(promise); shouldn't we remove the blocker even if it rejects? ::: browser/components/places/tests/unit/test_clearHistory_shutdown.js (Diff revision 1) > - do_test_pending(); > + yield startupPlaces(); If it's a one-time thing for this test, should be a util local to this test. Fwiw, the idea is that it should be able to shutdown properly even if it didn't complete initialization. I think that is working now. Is that impossible with async shutdown?

Comment on attachment 8573970 [details] MozReview Request: bz://1089695/Yoric https://reviewboard.mozilla.org/r/4969/#review4931

https://reviewboard.mozilla.org/r/4975/#review5017 > Is this a typo? I don't think you need to specify "class" here. > > Also, if I read this correctly, Database keeps alive DatabaseShutdown, that keeps alive Database... Who is breaking this cycle? > Might be worth to comment here about the cycle handling (basically, when this will be nullified) > Is this a typo? I don't think you need to specify "class" here. Well, since `DatabaseShutdown` is defined in the .cpp and this is a public header, I believe that I need to forward declare it somehow. > Also, if I read this correctly, Database keeps alive DatabaseShutdown, that keeps alive Database... Who is breaking this cycle? Oh, good catch. Fixed. > I'm assuming barrier and client are thread-safe, right? No, they are not, but `DatabaseShutdown` is main thread only. Is there a problem? > RECEIVED_STORAGE_COMPLETE? > There are too many completion callbacks here, let's try to distinguish naming. Good idea. > I'd really like to understand the difference between "notified observer" and "notified no service" here too... I kept the original behavior in which Places attempts to shutdown even if the observer service is not available. This enum holds the information on whether we could access the observer service or not, for forensics purposes. Not sure it's going to be useful, as I suspect we will never hit the branch in which there is no service, but if this is in the original code, I assume that there is a reason. > Either check service, or NS_ENSURE_TRUE it. I don't think we should crash in face of the user, even if we ignore an error condition. I'm not sure what you want me to do here. Barring an internal programming error, there should be no way this call can fail, so this is not going to cause crashes in face of the user. > client ? client.forget: nullptr; As you wish, but I'm not a big fan of this idea. If there is a failure here, it's a programming error, so I'd like to crash as soon as possible. > should we MOZ_ASSERT aParentClient? let's > null check mBarrier What's this going to bring us? Manual assertion crash instead of nsCOMPtr's built-in assertion crash? > Is this really useful? > The fact observer service may not be available is very rare As mentioned above, I suspect it's not going to be very useful. On the other hand, it doesn't really cost us anything. > We do we need to create a new comptr? shouldn't AddBlocker do that? Is this a try to static_cast with a hammer? :) Yes, it is :) What do you suggest? > null check profileBeforeChange As you wish, but we're just trading an assertion failure for another one. > MOZ_ASSERT(mConnectionShutdown) Same here. > This is likely bitrotted now, I merged the connections from Bookmarks and others into a single promise here. please merge with the new code The patch applied correctly. I'll keep an eye open for more subtle bitrot. > This should already exist in PlacesTestUtils.jsm... Ah, I don't think it existed when I first wrote my code :) > nit: the patch would be much smaller if your editor wouldn't try to fix every trailing space around :) Indirectly, it's your responsibility :) You convinced me to use template strings in `History.jsm`, which led me to change editor for JS code. I haven't found how to configure Atom to not fix trailing spaces yet.

https://reviewboard.mozilla.org/r/4973/#review5019 > will have to change this, due to new code in PlacesUtils. > > Also, bug 1091851 could be a good reason to unify all the paths... We need the wrappers to close before the main connection, in any case. I'm not sure I follow. What do you suggest I use?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #16) > Well, since `DatabaseShutdown` is defined in the .cpp and this is a public > header, I believe that I need to forward declare it somehow. well, just forward declare it at the top of the header file. > > I'm assuming barrier and client are thread-safe, right? > > No, they are not, but `DatabaseShutdown` is main thread only. > Is there a problem? DatabaseShutdown is decalred thread-safe. While it is main-thread only, I'm not sure it will be always destroyed on main-thread. Cause I'm not even sure Database that keeps it alive is destroyed on main-thread. > > I'd really like to understand the difference between "notified observer" and "notified no service" here too... > > I kept the original behavior in which Places attempts to shutdown even if > the observer service is not available. This enum holds the information on > whether we could access the observer service or not, for forensics purposes. while I think it's useful to keep shutting down, I don't think it's useful to report this error, cause it's very very very rare. > > Either check service, or NS_ENSURE_TRUE it. I don't think we should crash in face of the user, even if we ignore an error condition. > > I'm not sure what you want me to do here. Barring an internal programming > error, there should be no way this call can fail, so this is not going to > cause crashes in face of the user. How can you tell there's no way the call can fail? OOM? Is that call infallible? > As you wish, but I'm not a big fan of this idea. If there is a failure here, > it's a programming error, so I'd like to crash as soon as possible. Nope, things can go wrong for a lot of reasons, not just for a programming error, OOM, broken profiles... Things can cause services to come back null. > > null check mBarrier > > What's this going to bring us? Manual assertion crash instead of nsCOMPtr's > built-in assertion crash? That we don't crash. I prefer to crash in debug mode, but don't crash on the user. And since this might end up being null in some cases... > > We do we need to create a new comptr? shouldn't AddBlocker do that? Is this a try to static_cast with a hammer? :) > > Yes, it is :) > What do you suggest? does not static_cast work? I might look at it if you have issues. Eventually even at a later time when the patch is "ready" > > This is likely bitrotted now, I merged the connections from Bookmarks and others into a single promise here. please merge with the new code > > The patch applied correctly. I'll keep an eye open for more subtle bitrot. Strange, could be now we have 3 connections in PlacesUtils rather than 2.. We should merge them and just have one ReadOnly clone and one Wrapper. > You convinced me to use template strings in `History.jsm`, which led me to > change editor for JS code. I haven't found how to configure Atom to not fix > trailing spaces yet. Not sure how Atom works, Sublime Text has no issue with trailing spaces... Maybe there's an Atom extension to do that?

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #17) > https://reviewboard.mozilla.org/r/4973/#review5019 > > > will have to change this, due to new code in PlacesUtils. > > > > Also, bug 1091851 could be a good reason to unify all the paths... We need the wrappers to close before the main connection, in any case. > > I'm not sure I follow. What do you suggest I use? I'd like us to only have PromiseWrappedConnection, also for History.jsm... and maybe it should block Places shutdown... we need it to close before the main connection closes.

https://reviewboard.mozilla.org/r/4971/#review5021 > could you please remove Promise.jsm from here? Can we wait until we have proper error reporting in tests suites with DOM Promise? > You actually want a Map here? Well, I cannot attach a Map to a crash report, but I can attach a flat object. Do you mind if I keep a flat object? This will keep the code more concise. > if you clear your forensic map, you can't use if after the call to investigate anything... is this really needed? By "forensics", I mean "AsyncShutdown crash". It will be uploaded if we freeze at some point during sanitization. If we reach this line, sanitization is complete, so we don't eed the map anymore. > I think the idea here was to measure the total time taken to clear ALL items, but to me looks like you changed it to the time taken to clear each item? Indeed. Measuring total time doesn't make sense anymore once we are async. Measuring the total jank caused by sanitization, though, still works. Since both measures are the same pre-asyncification, I believe that this is the best path. > I wonder why we don't even log these errors to the console... Let's log them, then. > shouldn't we remove the blocker even if it rejects? Right. Note that, technically, we don't need to remove the blocker, as it will be removed automatically once it has completed. I don't know why I removed it, perhaps just to keep things tidy. > If it's a one-time thing for this test, should be a util local to this test. > Fwiw, the idea is that it should be able to shutdown properly even if it didn't complete initialization. I think that is working now. Is that impossible with async shutdown? I think it will work, but I must admit that haven't checked. Did we have any tests for this in the previous version?

https://reviewboard.mozilla.org/r/4975/#review5025 > No, they are not, but `DatabaseShutdown` is main thread only. > Is there a problem? > DatabaseShutdown is decalred thread-safe. While it is main-thread only, I'm not sure it will be always destroyed on main-thread. Cause I'm not even sure Database that keeps it alive is destroyed on main-thread. Ah, good point. Fixed. > I'm not sure what you want me to do here. Barring an internal programming error, there should be no way this call can fail, so this is not going to cause crashes in face of the user. > How can you tell there's no way the call can fail? OOM? Is that call infallible? Oh, you want me to defend against OOM during startup? Is there any way we can reach a state of Firefox that is better than a crash in case of OOM? > What's this going to bring us? Manual assertion crash instead of nsCOMPtr's built-in assertion crash? > That we don't crash. > I prefer to crash in debug mode, but don't crash on the user. And since this might end up being null in some cases... > As you wish, but I'm not a big fan of this idea. If there is a failure here, it's a programming error, so I'd like to crash as soon as possible. > Nope, things can go wrong for a lot of reasons, not just for a programming error, OOM, broken profiles... Things can cause services to come back null. Fair enough, this can happen. But, as above, I do not believe that we can do anything about it. Do you think there is any way we can recover from an error bad enough to prevent us from having some of our core services? I believe that a clean and quick crash is preferable to an entirely broken Firefox experience. > Yes, it is :) > What do you suggest? > does not static_cast work? I might look at it if you have issues. Eventually even at a later time when the patch is "ready" Ah, yes, it does. I'm not a big fan of `*_cast`, so I didn't try it. > Indirectly, it's your responsibility :) > > You convinced me to use template strings in `History.jsm`, which led me to change editor for JS code. I haven't found how to configure Atom to not fix trailing spaces yet. > Not sure how Atom works, Sublime Text has no issue with trailing spaces... Maybe there's an Atom extension to do that? Ok, I *think* I got it.

https://reviewboard.mozilla.org/r/4971/#review5033 > Well, I cannot attach a Map to a crash report, but I can attach a flat object. Do you mind if I keep a flat object? This will keep the code more concise. Done.

https://reviewboard.mozilla.org/r/4971/#review5023 > Can we wait until we have proper error reporting in tests suites with DOM Promise? Sure, but note we are using new promises everywhere already and we stopped using Promises.jsm in new Firefox code from some time. > Done. You can, [...mymap]. I prefer Maps to objects cause they are far more readable. but whatever. > Indeed. Measuring total time doesn't make sense anymore once we are async. Measuring the total jank caused by sanitization, though, still works. Since both measures are the same pre-asyncification, I believe that this is the best path. Then I'm not sure makes sense to keep the measurement cause I think most elements have their measure, plus we can't "re-use" a probe with completely different values... > I think it will work, but I must admit that haven't checked. > Did we have any tests for this in the previous version? Not a specific test, apart this one...

https://reviewboard.mozilla.org/r/4975/#review5113 > > How can you tell there's no way the call can fail? OOM? Is that call infallible? > > Oh, you want me to defend against OOM during startup? Is there any way we can reach a state of Firefox that is better than a crash in case of OOM? So, not a big fan, because I feel that we are obfuscating errors instead of craashing cleanly, but applying.

https://reviewboard.mozilla.org/r/4975/#review5347 I have been tracking down for a few days why I couldn't run test_removeVisitsByFilter.js to work without adding a `yield PlacesUtils.promiseDBConnection()`. Apparently, when attempting to instantiate `PlacesUtils.gAsyncDBConnPromised`, we clone `places.sqlite` before the file is created. Other files already exist at the time (`places.sqlite-shm` and `places.sqlite-wal`) but not `places.sqlite`. This causes sqlite3 to fail opening the db, which cascades into a mozStorage failure, a Sqlite.jsm failure, and a test failure. Any suggestion, mak?

https://reviewboard.mozilla.org/r/6547/#review5395 As you requested, here is the latest version, mak.

Comment on attachment 8573970 [details] MozReview Request: bz://1089695/Yoric /r/4971 - Bug 1089695 - Port sanitize dialog to History.jsm;r=mak /r/4973 - Bug 1076775 - Implement History.removeHistoryByFilter;r=mak /r/4975 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak /r/6533 - Bug 1043863 - Using AsyncShutdown to shutdown Places (applied feedback);r=mak /r/6535 - Bug 1076775 - Implement History.removeHistoryByFilter (feedback);r=mak /r/6537 - Bug 1089695 - Port sanitize dialog to History.jsm (feedback);r=mak /r/6539 - Bug 1043863 - Using AsyncShutdown to shutdown Places (applied feedback);r=mak /r/6541 - Bug 1089695 - Port sanitize dialog to History.jsm (feedback);r=mak /r/6543 - WIP /r/6545 - WIP - test_removeVisitsByFilter.js passes /r/6547 - WIP Pull down these commits: hg pull -r dcf3218df81213d171fa1f945270f73109e0bd57 https://reviewboard-hg.mozilla.org/gecko/

Comment on attachment 8573970 [details] MozReview Request: bz://1089695/Yoric https://reviewboard.mozilla.org/r/4969/#review5477 Pretty please, remove any change that is not strictly needed for this fix. It's becoming hard to review. For example the changes to Bookmarks.jsm or the trailing spaces changes, are really not worth the review effort due a bigger patch. I like the cleanups to sanitize since regardless we needed to refactor it, but not to other random stuff in the tree :) I don't care how you do that, edit diffs manually, send them by mail to me, dunno. Anything would be an improvement over this giant pile of diffs in RB... As I said on IRC, we can start landing SOME stuff, for example the API looks pretty much done, if tests pass, just ask for review over it and let's land it, even with no consumers! Then we can land async shutdown and finally the sanitize patch. Doing all of them at the same time is not helping us moving forward. ::: browser/base/content/sanitize.js (Diff revision 2) > + yield PlacesUtils.history.removeVisitsByFilter(range); please define the range inline: yield PlacesUtils.history.removeVisitsByFilter({ beginDate: new Date(this.range[0] / 1000), endDate: new Date(this.range[1] / 1000) }); ::: browser/base/content/sanitize.js (Diff revision 2) > - var os = Components.classes["@mozilla.org/observer-service;1"] > + let os = Components.classes["@mozilla.org/observer-service;1"] Since you're touching this, what about using Services.obs? ::: browser/components/places/tests/unit/test_clearHistory_shutdown.js (Diff revision 2) > - do_test_pending(); > + yield startupPlaces(); Remove this please? If there are still issues with this test we can look into them ::: storage/src/mozStorageConnection.cpp (Diff revision 2) > + printf_stderr("Connection::initialize failed %s: %d\n", Maybe this could be a good NS_WARNING ::: toolkit/components/places/Bookmarks.jsm (Diff revision 2) > -function* insertBookmark(item, parent) { > +let insertBookmark = Task.async(function*(item, parent) { what's the reason for this change? ::: toolkit/components/places/Bookmarks.jsm (Diff revision 2) > -function* fetchBookmark(info) { > +let fetchBookmark = Task.async(function*(info) { ditto ::: toolkit/components/places/Database.h (Diff revision 2) > + GetConnectionShutdown(); please oneline this. ::: toolkit/components/places/Bookmarks.jsm (Diff revision 2) > -function* fetchBookmarkByPosition(info) { > +let fetchBookmarkByPosition = Task.async(function*(info) { and so on, please stop the unrelated changes. It's making hard for me to review the patch. ::: toolkit/components/places/Database.cpp (Diff revision 2) > +#define TOPIC_SIMULATE_PLACES_MUST_CLOSE_2 "test-simulate-places-must-close-2" This is very confusing. Why do we need 2 notifications, and why they are defined in different places? It is also named differenctly from what the comment suggests? ::: toolkit/components/places/Database.cpp (Diff revision 2) > + } Since you don't want to use NS_ENSURE_SUCCESS please oneline all of these as if (NS_WARN_IF(NS_FAILED(rv) return rv; Yes, since these are all unexpected conditions, we should warn for all of them. that's why imo deprecating NS_ENSURE_SUCCESS rather than renaming it to NS_RETURN_IF_FAILED is a dumb idea, since in many cases we could debug issues only thanks to the pseudo stack generated by those warnings. This is valid for all the if (NS_FAILED(rv) { return rv; } that you added in the patch ::: toolkit/components/places/Database.cpp (Diff revision 2) > + } Why is this fine? ::: toolkit/components/places/Database.cpp (Diff revision 2) > + mDatabase = nullptr; Add comment that this is done to break a reference cycle. ::: toolkit/components/places/Database.cpp (Diff revision 2) > + } I'd prefer this to be non-fatal. If we return an error here for any reason all of Places will stop working, when the only actual issue would be that we can't block shutdown... Can we rather use ifs and MOZ_ASSERTS in this new code but don't early return? You can assert and warn as much as you wish. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > +Cu.import("resource://gre/modules/AppConstants.jsm", this); This fix is not needed, indeed it's already being fixed in another bug and will likely land in a few hours. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > + }, Please don't expose this through PlacesUtils. We don't want add-ons to hook into our shutdown process. that's why you properly exposed this in our private interface, we will never guarantee those are stable. I prefer if you type the whole chain every time. ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > - () => new Promise((resolve) => { > + () => Task.spawn(function*() { These changes are not needed ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > - () => new Promise((resolve) => { > + () => Task.spawn(function*() { ditto (not needed) ::: toolkit/components/places/PlacesUtils.jsm (Diff revision 2) > +XPCOMUtils.defineLazyGetter(this, "gAsyncRWDBConnPromised", () => { Should be removed ::: toolkit/components/places/nsNavHistory.cpp (Diff revision 2) > +#define TOPIC_SIMULATE_PLACES_MUST_CLOSE_1 "test-simulate-places-must-close-1" Sigh, I need an high level introduction to why we need 2 badly named topics... ::: toolkit/components/places/nsPIPlacesDatabase.idl (Diff revision 2) > */ Please while here change this comment, it must be clear that NO external consumers should use these for any reason. We don't guarantee anything about these API, they could change or disappear at any time. Only for internal usage. ::: toolkit/components/places/nsPIPlacesDatabase.idl (Diff revision 2) > + readonly attribute nsIAsyncShutdownClient shutdownClient; needs a brief javadoc ::: toolkit/components/places/tests/head_common.js (Diff revision 2) > + do_print("shutdownPlaces: sent test-simulate-places-must-close-2"); Yeah, I don't understand the need for both notifications. Plea

So, I tried to run tests with yout patch, and I didn't hit any of the failures you said regarding being unable to open the database... Did you try to Try run your patch? I suspect there's something suspicious at your filesystem level :) The only failures I see in toolkit tests are toolkit/components/places/tests/bookmarks/test_async_observers.js toolkit/components/places/tests/expiration/test_annos_expire_session.js and they look related to the shutdown simulation changes you did...

OK let me be clear on what I want. I want the removeByFilter API patch by itself in its own bug with a review request and a single diff. Then I want the async shutdown patch by itself in a bug with a review request and a single diff. Finally when those 2 landed, we will pick up the sanitize refactoring.

Note that I was *not* asking for a review. You asked me to upload a WIP patch, you got one :)

I know, but while there I figured wip comments would have been useful to drive the work.

But yes, I will have to take apart (again) all my patches and figure out which line belongs to which bug. Just please don't let them bitrot next time :)

Sorry, we're in a flux of changes due to new Bookmarks, keywords and transactions APIs. We're doing out best to keep disruption to a minimum.

Bug 1089695 - Async sanitize.js;r=mak

Comment on attachment 8613772 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r=mak

(sorry for the noise, I'm cleaning up the RB request now that the blocking bugs have landed)

https://reviewboard.mozilla.org/r/4971/#review5089 > Sure, but note we are using new promises everywhere already and we stopped using Promises.jsm in new Firefox code from some time. Yeah, and my patch that blocked error reporting for DOM Promise was blocked itself since October. But I hope to land it next week. > Then I'm not sure makes sense to keep the measurement cause I think most elements have their measure, plus we can't "re-use" a probe with completely different values... Well, the value will go down, but it still measures the same thing (at least one of the two possible definitions of the thing pre-patch).

Bug 1089695 - Async sanitize.js;r=mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak https://reviewboard.mozilla.org/r/9811/#review9587 Almost there, still something to look at.

https://reviewboard.mozilla.org/r/9809/#review9585 ::: browser/base/content/sanitize.js:4 (Diff revision 1) > -# file, You can obtain one at http://mozilla.org/MPL/2.0/. > +// file, You can obtain one at http://mozilla.org/MPL/2.0/. should likely use the /**/ form, there's no // form, see https://www.mozilla.org/MPL/headers/ ::: browser/base/content/sanitize.js:34 (Diff revision 1) > + this._itemsToClear = {}; Unfortunately adding stuff to the constructor won't help converting this to a module object (I'd like to avoid having to instantiate a sanitizer object) could you please look into an alternative that doesn't require an instance? ::: browser/base/content/sanitize.js:48 (Diff revision 1) > + aArg) please brace this, since it's multi-line. Also nit: please align the canClearItem arguments ::: browser/base/content/sanitize.js:120 (Diff revision 1) > + this._itemsToClear[itemName] = '`clear` not in item'; nit: please use double apices or templates for strings (Coding style) ::: browser/base/content/sanitize.js:218 (Diff revision 1) > + yield Promise.resolve(); // Don't block the main thread too long I'd probably move this after removeAll, inside the else, since in the other case we already yielded for a single removal. ::: browser/base/content/sanitize.js:209 (Diff revision 1) > + yield Promise.resolve(); // Don't block the main thread too long I suspect this might make cookies deletion take a much longer time.... Did you test the difference with many cookies? Also, be ready for a bunch of telemetry regression bugs filed blocking this... ::: browser/base/content/sanitize.js:565 (Diff revision 1) > + }); just this._clear(resolve)? ::: browser/base/content/sanitize.js:760 (Diff revision 1) > - ww.openWindow(null, // make this an app-modal window on Mac > + null: // make this an app-modal window on Mac add space before : ::: browser/base/content/sanitize.js:804 (Diff revision 1) > - }); > + }); nit: just move the if inside the then, so you can avoid promise = promise.then... Btw, I think in the startup case we should clear the pref, instead of letting browser.js do that http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1689 That means, if we clear the pref here, we can completely remove _initializeSanitizer (it is still trying to migrate 3.x prefs :/) . Regardless the sanitizer is initialized by nsBrowserGlue on final-ui-startup so there's no need for another initialization path in browser.js... ::: browser/base/content/sanitize.js:818 (Diff revision 1) > + shutdownClient.addBlocker("sanitize.js: Sanitize on " + (isStartup?"startup":"shutdown"), add spaces around ? and : ::: browser/base/content/newtab/grid.js:140 (Diff revision 1) > + Services.obs.notifyObservers(null, "browser:about-newtab-grid-refresh-complete", ""); This change looks unrelated to the patch? Please move it elsewhere ::: browser/base/content/test/newtab/browser_newtab_bug722273.js:19 (Diff revision 1) > + Task.spawn(function*() { shouldn't this return the task promise? As it is looks like the test is not waiting for anything Is it possible to change this to use add_task? ::: browser/base/content/test/newtab/browser_newtab_bug722273.js:40 (Diff revision 1) > +function promiseWhenGridUpdated() { Also this should likely move to a separate bug ::: browser/base/content/test/newtab/browser_newtab_bug722273.js:66 (Diff revision 1) > - handleError: function () ok(false, "couldn't add visit"), > + handleError: function () reject(new Error("Couldn't add visit")), This could maybe use PlacesTestUtils.addVisits?

https://reviewboard.mozilla.org/r/9809/#review9591 > Also this should likely move to a separate bug Well, making sanitize async broke this test. That's how I managed to fix it. > This could maybe use PlacesTestUtils.addVisits? I wanted to minimize interventions. Followup? > This change looks unrelated to the patch? Please move it elsewhere Actually, this was needed to make browser/base/content/test/newtab/browser_newtab_bug722273.js work. I just found another way, so removing this change. > I suspect this might make cookies deletion take a much longer time.... Did you test the difference with many cookies? > > Also, be ready for a bunch of telemetry regression bugs filed blocking this... Yes, I am expecting telemetry regressions, but then, rewriting the entire module to make it async will play havoc on Telemetry anyway. Fixed to make it happen less often. > just this._clear(resolve)? Actually, no because we need to handle `return false` as an error. But then, this doesn't make sense, so rewriting the entire method to make it behave correctly. > Unfortunately adding stuff to the constructor won't help converting this to a module object (I'd like to avoid having to instantiate a sanitizer object) > > could you please look into an alternative that doesn't require an instance? Done, but removing the need for a constructor will require doing something a little saner with how preferences are handled. The current implementation is just crazy. > nit: just move the if inside the then, so you can avoid promise = promise.then... > > Btw, I think in the startup case we should clear the pref, instead of letting browser.js do that > > http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1689 > > That means, if we clear the pref here, we can completely remove _initializeSanitizer (it is still trying to migrate 3.x prefs :/) . Regardless the sanitizer is initialized by nsBrowserGlue on final-ui-startup so there's no need for another initialization path in browser.js... Ok, I've tried to clean this up a bit. The more I look at the code of the sanitizer, the less I understand how it hasn't entirely collapsed yet. > shouldn't this return the task promise? As it is looks like the test is not waiting for anything > > Is it possible to change this to use add_task? I went for a different hack instead to make it work.

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r=mak

Fixing all of this turned harder than I thought, in part because I had forgotten to port some tests. In particular, to avoid an interesting memory leak, I needed to wrap sanitize.js in a jsm to ensure isolation between compartments.

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Bug 1089695 - Fixing wrong dependency in Places shutdown;r?mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r?mak

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2242188edd8d, btw.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak https://reviewboard.mozilla.org/r/13031/#review12215 ::: toolkit/components/places/PlacesUtils.jsm:2065 (Diff revision 2) > + let TOPIC = "places-connection-closed"; I don't think we should go further than places-will-close-connection, connection-closed is too late to do anything. ::: toolkit/components/places/PlacesUtils.jsm:2082 (Diff revision 2) > - "PlacesUtils read-only connection closing", > + Sqlite.shutdown.addBlocker("PlacesUtils read-only connection closing", I'm very confused, I thought the scope you added PlacesUtils.history.shutdownClient was exactly to build a proper chain of shutdown events, instead of doing the old "wait for notifications" thing. Are we moving backwards? With this patch PlacesUtils.history.shutdownClient is unused so looks like it's just pointless and/or broken? So, looks like the code before was blocking Places shutdown until Sqlite.jsm closed, why that doesn't work? maybe we are blocking Places shutdown too late, when it already started closing the connection? Effectively I'm not sure why we are calling BlockShutdown at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#1985 only for TOPIC_SIMULATE_PLACES_MUST_CLOSE_2, something is fishy here.

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak https://reviewboard.mozilla.org/r/9811/#review12219 ::: browser/base/content/sanitize.js:727 (Diff revisions 1 - 4) > +Sanitizer.PREF_SANITIZE_DID_SHUTDOWN = "privacy.sanitize.didShutdown"; please don't rename the pref (it was "didShutdownSanitize"), some add-ons are using it, and it will create confusion to users having it set to non-default value ::: browser/base/content/sanitize.js:183 (Diff revisions 1 - 4) > + // get a chance to complete. bad copy/pasted comment ::: browser/base/content/sanitize.js:258 (Diff revisions 1 - 4) > + yield new Promise(resolve => setTimeout(resolve, 0)); // Don't block the main thread too long ditto ::: browser/base/content/sanitize.js:250 (Diff revisions 1 - 4) > + yield new Promise(resolve => setTimeout(resolve, 0)); // Don't block the main thread too long I don't understand the need for setTimeout, are not promises always resolved on the next tick already? If so, why is not yield Promise.resolve(); enough? ::: browser/base/content/sanitizeDialog.js:10 (Diff revision 4) > +let Sanitizer = Cu.import("resource:///modules/Sanitizer.jsm", {}).Sanitizer; Just use defineLazyModuleGetter? Btw, I suspect your leak was related to passing itemsToClear. Instead of wrapping the whole thing into a module, that opens a can of worms in a bug that was not supposed to do that (you are handling the leak in sanitize.xul, but what about all of the other consumers, like preferences and add-ons?), did you try copying the input arguments? I think you might use cloneInto, or JSON.parse(JSON.stringify)) to obtain a copy of the input object for internal use. Also if the problem would be the progress object, you could do the same in async shutdown, copy the object. I don't think we should enter the modulification rabbit hole here, we are derailing already. ::: browser/base/content/sanitizeDialog.js:135 (Diff revision 4) > - .then(() => window.close()) > + setTimeout(() => s.sanitize(), 0); I think you are changing the dialog behavior here. we can't do this without UX intervention, cause we can't evaluate off-hand the cost this will have on users expectations. Please stay on track. ::: browser/base/content/test/general/browser_sanitizeDialog.js:44 (Diff revision 4) > + waitForAsyncUpdates(resolve); You can use PlacesTestUtils.promiseAsyncUpdates() ::: browser/base/content/test/general/browser_sanitizeDialog.js:855 (Diff revision 4) > finish(); This call to finish() cannot work anymore... you removed waitForExplicitFinish(); that means the test won't wait unless there's a pending promise Since nobody is checking the Task.spawn promise, this will just hang the test. ::: browser/base/content/test/general/browser_sanitizeDialog.js:876 (Diff revision 4) > + yield new Promise(resolve => setTimeout(resolve, 0)); why is this needed? a comment would be nice. ::: browser/base/content/test/general/browser_sanitizeDialog.js:875 (Diff revision 4) > - try { > + Task.spawn(function*() { The other thing I was thinking about is: since nobody is waiting for this Task promise, who guarantees that part of its code won't run after the test has officially finished and the harness is moving to the next one? I wonder if promiseClosed should be the Task promise... ::: browser/base/content/test/general/browser_sanitizeDialog.js:888 (Diff revision 4) > + args.AppendElement({ onDone: this._resolveDone }) Is this used anywhere? I can't find it. ::: browser/base/content/test/general/browser_sanitizeDialog.js:922 (Diff revision 4) > +function promiseObserve(topic) { There is already promiseTopicObserved in head.js ::: browser/base/content/test/newtab/browser_newtab_bug722273.js:14 (Diff revisions 1 - 4) > -function runTests() { > +add_task(function*() { could you please move the newtab related changes to a dependency bug and get r?tim

https://reviewboard.mozilla.org/r/13031/#review12215 > I'm very confused, I thought the scope you added PlacesUtils.history.shutdownClient was exactly to build a proper chain of shutdown events, instead of doing the old "wait for notifications" thing. Are we moving backwards? > With this patch PlacesUtils.history.shutdownClient is unused so looks like it's just pointless and/or broken? > > So, looks like the code before was blocking Places shutdown until Sqlite.jsm closed, why that doesn't work? maybe we are blocking Places shutdown too late, when it already started closing the connection? > Effectively I'm not sure why we are calling BlockShutdown at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#1985 only for TOPIC_SIMULATE_PLACES_MUST_CLOSE_2, something is fishy here. > I'm very confused, I thought the scope you added PlacesUtils.history.shutdownClient was exactly to build a proper chain of shutdown events, instead of doing the old "wait for notifications" thing. Are we moving backwards? Yes, the idea was to build a proper chain of shutdown events, but I got a dependency in the wrong order: Places shutdown should block `gAsyncDBConnectionPromised` shutdown, not the other way round. If we do it the other way round (as it was before this patch), we are closing the connection while some clients of Places may still be using it. Now, since we have JS code waiting on C++ code asynchronously, using `places-connection-closed` is the simplest way I could think of to achieve this. If you can think of something more elegant, I'm interested. > With this patch PlacesUtils.history.shutdownClient is unused so looks like it's just pointless and/or broken? Also note that `PlacesUtils.history.shutdownClient` is still very much useful, just for external clients, rather than internal clients. It is used by the patched sanitize.js. > Effectively I'm not sure why we are calling BlockShutdown at http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/Database.cpp#1985 only for TOPIC_SIMULATE_PLACES_MUST_CLOSE_2, something is fishy here. Well, as the name implies, we're _simulating_ shutdown. Otherwise, this method is called by AsyncShutdown.

https://reviewboard.mozilla.org/r/9811/#review12219 > I don't understand the need for setTimeout, are not promises always resolved on the next tick already? If so, why is not yield Promise.resolve(); enough? Not anymore with DOM Promise. They now use "microtasks", which are resolved "at the end of the current tick". > This call to finish() cannot work anymore... > > you removed waitForExplicitFinish(); that means the test won't wait unless there's a pending promise > Since nobody is checking the Task.spawn promise, this will just hang the test. Thanks for the spot. > Is this used anywhere? I can't find it. Oops, debugging artifact, thanks. > please don't rename the pref (it was "didShutdownSanitize"), some add-ons are using it, and it will create confusion to users having it set to non-default value My bad. > Just use defineLazyModuleGetter? > > Btw, I suspect your leak was related to passing itemsToClear. Instead of wrapping the whole thing into a module, that opens a can of worms in a bug that was not supposed to do that (you are handling the leak in sanitize.xul, but what about all of the other consumers, like preferences and add-ons?), did you try copying the input arguments? I think you might use cloneInto, or JSON.parse(JSON.stringify)) to obtain a copy of the input object for internal use. > > Also if the problem would be the progress object, you could do the same in async shutdown, copy the object. > > I don't think we should enter the modulification rabbit hole here, we are derailing already. > Just use defineLazyModuleGetter? I don't follow. What would this change? Plus we need to load the module during initialization of the dialog anyway. > Btw, I suspect your leak was related to passing itemsToClear. I spent a number of sleepless nights attempting to fix this. I tried changing the allocation of just about everything I could think of, as well as removing allocations altogether, and it didn't work, although admittedly, I didn't try `cloneInto` or `JSON.parse(JSON.stringify(...)`. My theory goes as follows: sanitize.js is loaded with the subscript loader, which I'm almost sure loads stuff in the same compartment as the caller. So any allocation in either script takes place in the dialog's compartment. Since closures tend to capture lots of data accidentally, any closure in the script can effectively keep the dialog alive until that closure is garbage-collected. Since the closure completion is delayed by any number of ticks, and its garbage-collection is non-deterministic, we end up holding a reference to the dialog at the end of the test. > you are handling the leak in sanitize.xul, but what about all of the other consumers, like preferences and add-ons? I figured we could wait for bug 1167238. > I think you are changing the dialog behavior here. we can't do this without UX intervention, cause we can't evaluate off-hand the cost this will have on users expectations. > > Please stay on track. Are you sure? If we don't do that, the patch actually degrades user-perceived performance. Since the dialog is modal, moving the work off the main thread without closing the dialog just increases the time until the user can use Firefox again. I don't feel we need to involve UX on something so basic.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #64) > Are you sure? If we don't do that, the patch actually degrades > user-perceived performance. Since the dialog is modal, moving the work off > the main thread without closing the dialog just increases the time until the > user can use Firefox again. I don't feel we need to involve UX on something > so basic. The fact is that users are used to this behavior: 1. clear 2. the dialog goes away 3. everything is clean now I'm sure people will clear, then notice stuff still appears in the awesomebar, or the downloads panel, or wherever else. They will just think the clear feature is broken. It's not a trivial thing to get this right. The only thing we made async here is history, I wonder if the problem is not the various setTimeout you added that are making the whole thing too slow? How longer does it take with just the history change and without the setTimeout? I wonder if you're trying to fix too much here... If we're just adding a few seconds we should probably do nothing differently, but have a bug for UX to clarify clearing is still in progress, if we close the dialog before.

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #64) > > Just use defineLazyModuleGetter? > > I don't follow. What would this change? it achieves the same as Cu.import(..., {}).Something and is more readable. > > you are handling the leak in sanitize.xul, but what about all of the other consumers, like preferences and add-ons? > > I figured we could wait for bug 1167238. Sure, the fact is indeed we should wait for bug 1167238 to have a module at all... Provided we can solve the leak differently. If there's no other way to solve the leak, then your solution will do and we'll fix the rest in bug 1167238

https://reviewboard.mozilla.org/r/9811/#review12219 > Not anymore with DOM Promise. They now use "microtasks", which are resolved "at the end of the current tick". Interesting, I thought even with microtasks they were still going back to the thread loop. Good to know. > > Just use defineLazyModuleGetter? > > I don't follow. What would this change? Plus we need to load the module during initialization of the dialog anyway. > > > Btw, I suspect your leak was related to passing itemsToClear. > > I spent a number of sleepless nights attempting to fix this. I tried changing the allocation of just about everything I could think of, as well as removing allocations altogether, and it didn't work, although admittedly, I didn't try `cloneInto` or `JSON.parse(JSON.stringify(...)`. > > My theory goes as follows: sanitize.js is loaded with the subscript loader, which I'm almost sure loads stuff in the same compartment as the caller. So any allocation in either script takes place in the dialog's compartment. Since closures tend to capture lots of data accidentally, any closure in the script can effectively keep the dialog alive until that closure is garbage-collected. Since the closure completion is delayed by any number of ticks, and its garbage-collection is non-deterministic, we end up holding a reference to the dialog at the end of the test. > > > you are handling the leak in sanitize.xul, but what about all of the other consumers, like preferences and add-ons? > > I figured we could wait for bug 1167238. I suggested defineLazyModuleGetter just because it's more readable then Cu.import(..., {}).Something and achieves the same. Regarding the module thing, the fact is indeed we should wait for bug 1167238 to have a module at all... Provided we can solve the leak differently. If there's no other way to solve the leak, then your solution will do and we'll fix the rest in bug 1167238 > Are you sure? If we don't do that, the patch actually degrades user-perceived performance. Since the dialog is modal, moving the work off the main thread without closing the dialog just increases the time until the user can use Firefox again. I don't feel we need to involve UX on something so basic. The fact is that users are used to this behavior: 1. clear 2. the dialog goes away 3. everything is clean now I'm sure people will clear, then notice stuff still appears in the awesomebar, or the downloads panel, or wherever else. They will just think the clear feature is broken. It's not a trivial thing to get this right. The only thing we made async here is history, I wonder if the problem is not the various setTimeout you added that are making the whole thing too slow? How longer does it take with just the history change and without the setTimeout? I wonder if you're trying to fix too much here... If we're just adding a few seconds we should probably do nothing differently, but have a bug for UX to clarify clearing is still in progress, if we close the dialog before.

https://reviewboard.mozilla.org/r/9811/#review12219 > I suggested defineLazyModuleGetter just because it's more readable then Cu.import(..., {}).Something and achieves the same. > > Regarding the module thing, the fact is indeed we should wait for bug 1167238 to have a module at all... Provided we can solve the leak differently. If there's no other way to solve the leak, then your solution will do and we'll fix the rest in bug 1167238 > I suggested defineLazyModuleGetter just because it's more readable then Cu.import(..., {}).Something and achieves the same. YMMV. Personally, I think it's more convoluted and less readable. Tell me if you want me to switch to `defineLazyModuleGetter`, but by default, I prefer what I wrote. > Regarding the module thing, the fact is indeed we should wait for bug 1167238 to have a module at all... Provided we can solve the leak differently. If there's no other way to solve the leak, then your solution will do and we'll fix the rest in bug 1167238 Well, the reason I put this micro-module is because I didn't find any other solution. > The fact is that users are used to this behavior: > 1. clear > 2. the dialog goes away > 3. everything is clean > > now I'm sure people will clear, then notice stuff still appears in the awesomebar, or the downloads panel, or wherever else. They will just think the clear feature is broken. It's not a trivial thing to get this right. > > The only thing we made async here is history, I wonder if the problem is not the various setTimeout you added that are making the whole thing too slow? How longer does it take with just the history change and without the setTimeout? I wonder if you're trying to fix too much here... If we're just adding a few seconds we should probably do nothing differently, but have a bug for UX to clarify clearing is still in progress, if we close the dialog before. Note that I haven't actually checked whether it's slower. It just makes sense because every time we have moved things off the main thread, we have traded responsiveness for throughput. And with a modal dialog, we don't get the benefits of responsiveness. So, what do you suggest? Rollback to the previous dialog-closing behavior, then file a UX bug?

https://reviewboard.mozilla.org/r/9811/#review12219 > > I suggested defineLazyModuleGetter just because it's more readable then Cu.import(..., {}).Something and achieves the same. > > YMMV. Personally, I think it's more convoluted and less readable. Tell me if you want me to switch to `defineLazyModuleGetter`, but by default, I prefer what I wrote. > > > Regarding the module thing, the fact is indeed we should wait for bug 1167238 to have a module at all... Provided we can solve the leak differently. If there's no other way to solve the leak, then your solution will do and we'll fix the rest in bug 1167238 > > Well, the reason I put this micro-module is because I didn't find any other solution. the modulegetter was mostly a nit. Well, you said you didn't try to clone argument objects. > Note that I haven't actually checked whether it's slower. It just makes sense because every time we have moved things off the main thread, we have traded responsiveness for throughput. And with a modal dialog, we don't get the benefits of responsiveness. > > So, what do you suggest? Rollback to the previous dialog-closing behavior, then file a UX bug? I think we should measure if the difference is noticeable, if it's not go back to the previous behavior and file a UX bug. If it's noticeable, change the behavior but still file a UX bug to notify we are changing the behavior and that might not be the best UI for our users.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r?mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

https://reviewboard.mozilla.org/r/9811/#review12219 > I think we should measure if the difference is noticeable, if it's not go back to the previous behavior and file a UX bug. If it's noticeable, change the behavior but still file a UX bug to notify we are changing the behavior and that might not be the best UI for our users. Experimenting with a loaded profile indicates that if we close the dialog and try to open the Bookmarks menu before sanitization is complete, we end up with an insanely long beachball (6+ minutes). I haven't found anything interesting with the profiler, http://people.mozilla.org/~bgirard/cleopatra/#report=9db62544362cd867923b8faa71a95eb23edfb14d&jankOnly=true&javascriptOnly=true . Bottom line: food for a followup bug, and for the moment, I won't autoclose the dialog. > the modulegetter was mostly a nit. > > Well, you said you didn't try to clone argument objects. Tried cloning, without success.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak https://reviewboard.mozilla.org/r/13031/#review12983 ::: toolkit/components/places/PlacesUtils.jsm:2065 (Diff revision 3) > + let TOPIC = "places-connection-closed"; As I said, this is too late, the wrappers must be closed before the main connection is closed, otherwise we will again issue warnings about unfinalized statements held by the wrappers... we should use places-will-close-connection ::: toolkit/components/places/PlacesUtils.jsm:2083 (Diff revision 3) > - conn.close.bind(conn)); > + Task.async(function*() { I'd prefer to avoid the Task here and just use a simple .then() chain. ::: toolkit/components/places/PlacesUtils.jsm:2108 (Diff revision 3) > - conn.close.bind(conn)); > + Task.async(function*() { ditto

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak https://reviewboard.mozilla.org/r/9811/#review13015 it ::: browser/base/content/sanitize.js:4 (Diff revision 5) > -# file, You can obtain one at http://mozilla.org/MPL/2.0/. > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ Unfortunately the patch is heavily bitrotted on sanitize.js, please unbitrot it. there's some change to cookies but some of the added code is basically the same you did here for all items so it can just be removed ::: browser/base/content/sanitize.js:103 (Diff revision 5) > + }) Sorry, but this is sort-of unreadable, please make: () => { return { progress }; } ::: browser/base/content/sanitize.js:112 (Diff revision 5) > + _sanitize: Task.async(function*(aItemsToClear, progress = {}) { add newline above method ::: browser/base/content/sanitize.js:283 (Diff revision 5) > + } with the new code clearing plugins in a callback this may not be needed... ::: browser/base/content/sanitizeDialog.js:11 (Diff revision 5) > +let {setTimeout} = Cu.import("resource://gre/modules/Timer.jsm", {}); doesn't look like you are using setTimeout anymore ::: browser/base/content/sanitizeDialog.js:56 (Diff revision 5) > - if (!aCanClear) { > + then(canClear => { please refactor as let promise = s.promise...().then(canClear => { ... }); tasks.push(promise); ::: browser/base/content/test/plugins/browser_clearplugindata.js:87 (Diff revision 5) > - sanitizer.sanitize(); > + yield sanitizer.sanitize(); this has already been done and changes to this file can likely go ::: browser/base/content/sanitizeDialog.js:137 (Diff revision 5) > - .then(null, Components.utils.reportError); > + .then(null, Components.utils.reportError); these indentation changes are wrong ::: browser/base/content/test/general/browser_sanitizeDialog.js:179 (Diff revision 5) > + wh = null; is this wh = null debug leftover? ::: browser/base/content/test/general/browser_sanitizeDialog.js:674 (Diff revision 5) > - } > + return wh.promiseCLosed; typo: CLosed ::: browser/base/content/test/general/browser_sanitizeDialog.js:691 (Diff revision 5) > + this.promiseClosed = new Promise(resolve => {this._resolveClosed = resolve}); nit: this may just use PromiseUtils.defer() ::: browser/base/content/test/general/browser_sanitizeDialog.js:874 (Diff revision 5) > + yield new Promise(resolve => waitForAsyncUpdates(resolve)); iirc we have promiseAsyncUpdates (in PlacesTestUtils?) it looks mostly good! I'd like to take a final look to an unbitrotted patch, but should be an easy r+

https://reviewboard.mozilla.org/r/9811/#review13015 > with the new code clearing plugins in a callback this may not be needed... I can remove it, your call. > nit: this may just use PromiseUtils.defer() Sure. Now that I have something that works, I'd like to stop tinkering with it, in case it blows :)

Sigh. Unbitrotting and/or applying feedback causes tests to fail in subtle ways, once again. Will investigate.

https://reviewboard.mozilla.org/r/9811/#review12219 > The other thing I was thinking about is: since nobody is waiting for this Task promise, who guarantees that part of its code won't run after the test has officially finished and the harness is moving to the next one? > I wonder if promiseClosed should be the Task promise... Well, that's why `promiseClosed` is resolved at the end of the Task. Nothing else guarantees that. > why is this needed? a comment would be nice. Actually, it's not necessary.

https://reviewboard.mozilla.org/r/13031/#review12215 > I don't think we should go further than places-will-close-connection, connection-closed is too late to do anything. Actually, both seem to be wrong. 1. We need to make sure that we don't close the connection while there could be any clients left. So that means that we should close _after_ `places-will-close-connection`. 2. We need to close before the underlying mozStorage connection, which currently takes place in the same tick as `places-will-close-connection`. This sounds like we need one of two things: - either add a second shutdown client for internal users of Places (which is essentially what we do in Sqlite.jsm); - or add yet another `places-currently-closing-connection`. I'll need to think it through.

https://reviewboard.mozilla.org/r/13031/#review12215 > Actually, both seem to be wrong. > > 1. We need to make sure that we don't close the connection while there could be any clients left. So that means that we should close _after_ `places-will-close-connection`. > 2. We need to close before the underlying mozStorage connection, which currently takes place in the same tick as `places-will-close-connection`. > > This sounds like we need one of two things: > - either add a second shutdown client for internal users of Places (which is essentially what we do in Sqlite.jsm); > - or add yet another `places-currently-closing-connection`. > > I'll need to think it through. There is only one consumer of places-will-close-connection and it's expiration that doesn't use the wrappers. IT's an internal consumer and it does that for very specific shutdown ordering reasons (we might be able to fix that with async shutdown I guess). It can continue using direct access to the underlying connection. We should just update the wrappers documentation stating that they cannot be used after places-shutdown, that is the common rule for everything using Places, expiration is just a special exception.

https://reviewboard.mozilla.org/r/13031/#review12215 > There is only one consumer of places-will-close-connection and it's expiration that doesn't use the wrappers. IT's an internal consumer and it does that for very specific shutdown ordering reasons (we might be able to fix that with async shutdown I guess). It can continue using direct access to the underlying connection. > We should just update the wrappers documentation stating that they cannot be used after places-shutdown, that is the common rule for everything using Places, expiration is just a special exception. Right. I was misled by the fact that several tests depend on that notification.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r?mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

https://reviewboard.mozilla.org/r/13031/#review12983 > I'd prefer to avoid the Task here and just use a simple .then() chain. Dropping that one because the Task really makes the state clearer.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r?mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Bug 1089695 - Async sanitize.js (newtab tests);r?tim

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak https://reviewboard.mozilla.org/r/13031/#review15517 Ship It! ::: toolkit/components/places/PlacesUtils.jsm:55 (Diff revision 4) > + "resource://gre/modules/Timer.jsm"); I think this is not needed? I don't see what is using it in this diff... Maybe it's elsewhere?

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak https://reviewboard.mozilla.org/r/9811/#review15523 ::: browser/base/content/test/general/browser_sanitizeDialog.js:871 (Diff revisions 4 - 6) > - yield new Promise(resolve => setTimeout(resolve, 0)); > +// yield new Promise(resolve => setTimeout(resolve, 0)); leftover? ::: browser/base/content/sanitize.js:297 (Diff revision 6) > - // If the plugin doesn't support clearing by age, clear everything. > + // If the plugin doesn't support clearing by age, clear everything. the comment now is a bit confusing with the following if, I think i'd prefer to invert it like if (rv == ...NOT_SUPPORTED) { yield... } so that it reads like the comment above it. ::: browser/base/content/sanitize.js:359 (Diff revision 6) > - > + console.error(`Error while resetting the predictor`, e); there's no reason to use a template string here ::: browser/base/content/sanitize.js:481 (Diff revision 6) > + passwords: { passwords clear has been deprecated by Bug 1102184, looks like here you have a merge problem and thus re-introducing removed code. ::: browser/base/content/sanitize.js:573 (Diff revision 6) > var push = Cc["@mozilla.org/push/NotificationService;1"] likely bitrotted by http://hg.mozilla.org/mozilla-central/rev/9a5f4793b17f pay attention to the merge ::: browser/base/content/sanitize.js:837 (Diff revision 6) > - > + if (Preferences.has(Sanitizer.PREF_SANITIZE_DID_SHUTDOWN)) { you wrongly removed the changes done by http://hg.mozilla.org/mozilla-central/rev/fb4f42bbeb01 (see "one time migration") ::: browser/components/nsBrowserGlue.js:170 (Diff revision 6) > + "resource://gre/modules/AppConstants.jsm"); looks like it's unused?

https://reviewboard.mozilla.org/r/9811/#review15523 > likely bitrotted by http://hg.mozilla.org/mozilla-central/rev/9a5f4793b17f pay attention to the merge Thanks for the heads up. Side-note: that code calls `dump`? Weird. > looks like it's unused? I don't remember. Anyway, another patch put this in nsBrowserGlue.js.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert Bug 1089695 - Async sanitize.js (newtab tests);r?ttaubert

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r?mak

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert Bug 1089695 - Async sanitize.js (newtab tests);r?ttaubert

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak https://reviewboard.mozilla.org/r/9811/#review15607 ::: browser/base/content/sanitize.js:9 (Diff revision 7) > + "resource://gre/modules/AppConstants.jsm"); I assume your scope was to stop preprocessing sanitize.js, though you didn't remove the * from the jar.mn file

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r=mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r=mak

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert Bug 1089695 - Async sanitize.js (newtab tests);r?ttaubert

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert Bug 1089695 - Async sanitize.js (newtab tests);r?ttaubert

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert https://reviewboard.mozilla.org/r/14987/#review15845

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aceb60196c25

https://hg.mozilla.org/integration/fx-team/rev/78c891ba5de1 https://hg.mozilla.org/integration/fx-team/rev/6b768986595f https://hg.mozilla.org/integration/fx-team/rev/b2c46fdeca8b

Backed out for frequent Marionette crashes. https://treeherder.mozilla.org/logviewer.html#?job_id=4470829&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/764e07ab1ad0

Thanks.

I haven't been able to reproduce on Mac yet. Is that on all platforms?

It was mostly on Windows from what I was seeing. https://treeherder.mozilla.org/#/jobs?repo=fx-team&filter-searchStr=mn&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&fromchange=1daf4a74ada5&tochange=764e07ab1ad0

mak, I can reproduce the crash if I rebase my patch on top of the latest m-c, but with a slightly older m-c, I couldn't reproduce. Do you have any idea of a recent patch that could be involved such crashes?

off-hand no, I guess the best idea would be to use mozregression...

I finally managed to extract stacks. Assertion failure: https://dxr.mozilla.org/mozilla-central/source/storage/mozStorageAsyncStatementExecution.cpp?from=mozStorageAsyncStatementExecution.cpp&offset=0#180-185 accompanied by the following comment: // If we don't have a valid target, this is a bug somewhere else. In the past, // this assert found cases where a Run method would schedule a new statement // without checking if asyncClose had been called. The caller must prevent // that from happening or, if the work is not critical, just avoid creating // the new statement during shutdown. See bug 718449 for an example. C++ stack: application crashed [@ mozilla::storage::AsyncExecuteStatements::execute(nsTArray<mozilla::storage::StatementData>&, mozilla::storage::Connection*, sqlite3*, mozIStorageStatementCallback*, mozIStoragePendingStatement**)] XUL!mozilla::storage::AsyncExecuteStatements::execute(nsTArray<mozilla::storage::StatementData>&, mozilla::storage::Connection*, sqlite3*, mozIStorageStatementCallback*, mozIStoragePendingStatement**) [mozStorageAsyncStatementExecution.cpp:59062d51df42 : 185 + 0x0] XUL!mozilla::storage::StorageBaseStatementInternal::ExecuteAsync(mozIStorageStatementCallback*, mozIStoragePendingStatement**) [StorageBaseStatementInternal.cpp:59062d51df42 : 197 + 0x12] Called by JS stack: 1 ConnectionData.prototype<.executeCached/<(resolve = [function], reject = [function]) ["resource://gre/modules/Sqlite.jsm":479] this = [object Object] 2 ConnectionData.prototype<.executeCached(sql = "INSERT INTO moz_bookmarks (fk, type, parent, position, title, dateAdded, lastModified, guid) VALUES ((SELECT id FROM moz_places WHERE url = :url), :type, :parent, :index, :title, :date_added, :last_modified, :guid) ", params = [object Object]) ["resource://gre/modules/Sqlite.jsm":477] this = [object Object] 3 ConnectionData.prototype<.executeBeforeShutdown/loggedDb<.executeCached.value<() ["resource://gre/modules/Sqlite.jsm":354] this = [object Object] 4 next(val = undefined) ["self-hosted":656] this = [object Generator] 5 TaskImpl_run(aSendResolved = true) ["resource://gre/modules/Task.jsm":314] this = [object Object] 6 TaskImpl(iterator = [object Generator]) ["resource://gre/modules/Task.jsm":275] this = [object Object] 7 createAsyncFunction/asyncFunction("INSERT INTO moz_bookmarks (fk, type, parent, position, title, dateAdded, lastModified, guid) VALUES ((SELECT id FROM moz_places WHERE url = :url), :type, :parent, :index, :title, :date_added, :last_modified, :guid) ", [object Object]) ["resource://gre/modules/Task.jsm":249] this = [object Object] 8 transaction() ["resource://gre/modules/Bookmarks.jsm":798] 9 InterpretGenerAbout to crash, dumping JS stack This is the transaction from `insertBookmark`. The crash takes place during shutdown. Before crashing, we get the following warnings: [2928] WARNING: SQL statement 'SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', b.dateAdded, b.lastModified, b.type, b.title, h.url AS url, b.id AS _id, b.parent AS _parentId, (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount, p.parent AS _grandParentId FROM moz_bookmarks b LEFT JOIN moz_bookmarks p ON p.id = b.parent LEFT JOIN moz_places h ON h.id = b.fk WHERE p.guid = :parentGuid AND b.position = IFNULL(:index, (SELECT count(*) - 1 FROM moz_bookmarks WHERE parent = p.id)) ' (6abf3b20) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'SELECT b.id, b.guid from moz_bookmarks b WHERE b.guid = :guid LIMIT 1' (793dc8d0) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'WITH RECURSIVE ancestors(aid) AS ( SELECT id FROM moz_bookmarks WHERE guid = :guid UNION ALL SELECT parent FROM moz_bookmarks JOIN ancestors ON id = aid WHERE type = :type ) UPDATE moz_bookmarks SET lastModified = :time WHERE id IN ancestors ' (793dc2e0) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'INSERT INTO moz_bookmarks (fk, type, parent, position, title, dateAdded, lastModified, guid) VALUES ((SELECT id FROM moz_places WHERE url = :url), :type, :parent, :index, :title, :date_added, :last_modified, :guid) ' (793dc1b0) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'UPDATE moz_bookmarks SET position = position + 1 WHERE parent = :parent AND position >= :index ' (793dbf50) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'INSERT OR IGNORE INTO moz_places (url, rev_host, hidden, frecency, guid) VALUES (:url, :rev_host, 0, :frecency, GENERATE_GUID()) ' (793dbbc0) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'SELECT GENERATE_GUID() AS guid' (793da660) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 [2928] WARNING: SQL statement 'SELECT b.guid, IFNULL(p.guid, "") AS parentGuid, b.position AS 'index', b.dateAdded, b.lastModified, b.type, b.title, h.url AS url, b.id AS _id, b.parent AS _parentId, (SELECT count(*) FROM moz_bookmarks WHERE parent = b.id) AS _childCount, p.parent AS _grandParentId FROM moz_bookmarks b LEFT JOIN moz_bookmarks p ON p.id = b.parent LEFT JOIN moz_places h ON h.id = b.fk WHERE b.guid = :guid ' (793dac50) should have been finalized before closing the connection: file c:/builds/moz2_slave/try-w64-d-00000000000000000000/build/src/storage/mozStorageConnection.cpp, line 1009 Does any of this ring a bell, mak?

Ok, I think I found the issue. I'll try and publish an updated patch today.

Comment on attachment 8632410 [details] MozReview Request: Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak Bug 1089695 - Fixing wrong dependency in Places shutdown;r=mak

Comment on attachment 8616701 [details] MozReview Request: Bug 1089695 - Async sanitize.js;r=mak Bug 1089695 - Async sanitize.js;r=mak

Comment on attachment 8648734 [details] MozReview Request: Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert Bug 1089695 - Async sanitize.js (newtab tests);r=ttaubert

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48a1dda906a3

https://hg.mozilla.org/integration/fx-team/rev/b8f3cba5419f https://hg.mozilla.org/integration/fx-team/rev/9a6d45264eb2 https://hg.mozilla.org/integration/fx-team/rev/bbefa0967bad

https://hg.mozilla.org/mozilla-central/rev/b8f3cba5419f https://hg.mozilla.org/mozilla-central/rev/9a6d45264eb2 https://hg.mozilla.org/mozilla-central/rev/bbefa0967bad

Since this caused the unexpected deletion of all logins for some users (bug 1242176), Ritu suggested that we get QE verification that the refactoring didn't introduce other data loss bugs with sanitization before we release a hotfix for passwords. Florin, can you get someone to do testing of the "sanitization" aka. "Clear on Shutdown" feature in Fx44 to ensure there is no unexpected data loss of other data types. See https://support.mozilla.org/en-US/kb/delete-browsing-search-download-history-firefox#w_how-do-i-make-firefox-clear-my-history-automatically and perform steps 1-7 followed by unchecking the box in step 4 which should cause the "delete data on shutdown" feature to be off. Then we need to verify that data for all of the types in step 6 aren't deleted after a shutdown. Thinking about it a bit, I don't think "Active Logins" makes much sense in that dialog as it refers to HTTP authentication session which I thought were always ended upon shutdown but perhaps I'm wrong and session restore persists them…

Note that you can check HTTP cache usage in about:cache

We're currently looking into this, we'll follow up with our test result as soon as possible.

We managed to verify this issue on Firefox 44.0 build 3 across platforms[1] and is no longer reproducible. While testing, we've encountered an issue related to the 'Active Logins' option, described in https://bugzilla.mozilla.org/show_bug.cgi?id=644523 . For more details about the tests, please see https://public.etherpad-mozilla.org/p/1089695. [1]Windows 7 x64, Windows 10x86, Mac OS X 10.9.5, Ubuntu 14.04 x64

Hi guys, so I tried the following two scenarios (A&B) on 45.0b1 (which I presume has the same bug as Fx44): Scenario A: 1. Under options -> privacy -> checked "Clear history when Firefox closes". 2. Clicked "Settings" button. 3. On "Settings for Clearing History", uncheck everything and only checked "Cache". 4. Closed that dialog and unchecked "Clear history when Firefox closes" Under options -> privacy. 5. Before closing the browser, I did about:cache and under "memory" I saw 840 entries and disk 14147 entries. 6. Alt+F -> exit on browser. 7. Reopened browser and checked about: cache. In about: cache, I saw under "memory" 3 entries and under "disk" ~14155 (14437, 14443) entries. Scenario B: Same steps as scenario A except skipped step #4. This time in about:cache, for "disk" I only saw 36 entries (which means the cache got cleared?). This makes me feel that we do *not* have the same dataloss issue in "cache" history entity as "passwords". Matt, can you please confirm that my scenarios and result verification makes sense? Thanks!

(In reply to Ritu Kothari (:ritu) from comment #125) > Matt, can you please confirm that my > scenarios and result verification makes sense? Thanks! That makes sense for cache.

(In reply to Mihai Boldan, QA [:mboldan] from comment #124) > While testing, we've encountered an issue related to the 'Active Logins' > option, described in https://bugzilla.mozilla.org/show_bug.cgi?id=644523 . I mentioned the issue with Active Logins in my instructions: (Quoting Matthew N. [:MattN] from comment #121) > Thinking about it a bit, I don't think "Active Logins" makes much sense in > that dialog as it refers to HTTP authentication session which I thought were > always ended upon shutdown but perhaps I'm wrong and session restore > persists them… You mentioned on IRC that you were testing mozillazine but that's not HTTP authentication, that's form authentication. You would need to test with something like http://httpbin.org/basic-auth/user/passwd (user:passwd).

I confirm the same behavior with the http://httpbin.org/basic-auth/user/passwd site as with mozillazine we tested on Friday. Tested with Firefox 44 RC build3 on Windowos10 x86.

I backed out this from beta, to reduce the number of crashes due to timeout aborts, in bug 1248489 https://hg.mozilla.org/releases/mozilla-beta/rev/30d48874cea4 Since there's no "backout" flag, I'm marking it as disabled. Aurora 46 was untouched and still has this fix, we'll work on a more "proper" fix for it.