Closed Bug 1043863 Opened 10 years ago Closed 9 years ago

[AsyncShutdown] Use AsyncShutdown for shutting down Places

Categories

(Toolkit :: Places, defect)

Product:
Toolkit
Component:
Places
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla41
Tracking Flags:
Tracking Status
firefox41 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(3 obsolete files) 

      No description provided.
Assignee: nobody → dteller
Please stop the async shutdown train until We discuss it, it's not blocking any change We currently plan and it should not block them.
This is expensive and the outcome is uncertain, let's not lose our focus.
To be clear, We should implement RemovePlaces, not this.
I'm not sure I understand the reasoning. Having asynchronous Places operations running (and potentially interrupted) during shutdown sounds like a Bad Thing. Let's chat once you're back.
For what it's worth, I have an advanced prototype of making History shutdown-safe, that passes many (but not all) tests.
Side-note: at the moment, shutdown-safety of Places is ensured through a mutex that stops the main thread from shutting down until all ongoing off main thread operations are complete. In edge cases, I believe that this mutex may stop the main thread for several seconds during shutdown.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #5)
> Side-note: at the moment, shutdown-safety of Places is ensured through a
> mutex that stops the main thread from shutting down until all ongoing off
> main thread operations are complete. In edge cases, I believe that this
> mutex may stop the main thread for several seconds during shutdown.

The mutex only serves some history requests that are not critical, it is used to avoid starting work after shutdown, not to block already started work.
Blocks: 1076775
Blocks: 1091851
Attached file MozReview Request: bz://1043863/Yoric (obsolete) — 
/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - AsyncShutdown for e10s;r=froydnj

Pull down these commits:

hg pull -r e936bab5a803f47cad7302b3310051c00a8108fc https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596482 - Flags: review?(nfroyd)
Attachment #8596482 - Flags: review?(mak77)
https://reviewboard.mozilla.org/r/7485/#review6265

From a pure code perspective, this looks fine.  However, I don't see anything using the new barrier in this bug, the blocked bugs, or the dependent bugs, so I'm a bit uncertain about why we're doing this change in this bug.  Can you explain?

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm:943
(Diff revision 1)
> +// Parent process

Is this comment (and the one below) referring to whether the barriers are only available in the given process?  (If this is the case, shouldn't we take steps to only expose the appropriate barriers in the corresponding process?) Or are the comments specifying which process the barriers pertain to, even though all the barriers are actually for the parent process (IIUC)?  Either way, I think these comments could stand a little elaboration.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo" - away until May 6th) from comment #11)
> https://reviewboard.mozilla.org/r/7485/#review6279
> 
> Look at https://reviewboard.mozilla.org/r/7483/diff/0 (the other patch of
> the same bug), we're using it in Database.cpp.

Sorry!  Was thinking too much about the Javascript side of things and not enough about the C++ side.
https://reviewboard.mozilla.org/r/7485/#review6293

> Is this comment (and the one below) referring to whether the barriers are only available in the given process?  (If this is the case, shouldn't we take steps to only expose the appropriate barriers in the corresponding process?) Or are the comments specifying which process the barriers pertain to, even though all the barriers are actually for the parent process (IIUC)?  Either way, I think these comments could stand a little elaboration.

Good idea. I suspect that this will need some fixes to the content process, though, so if you don't mind, I'd like to keep this as a followup bug.
I'll review this on Monday, today I don't have enough contiguous time for a deep review.
But I'll try to look at bug 1154877 in spare time, so maybe we can get that fixed soon too.
https://reviewboard.mozilla.org/r/7485/#review6377

Indeed!  Good to know we're not committed things without actual uses. :)

> Good idea. I suspect that this will need some fixes to the content process, though, so if you don't mind, I'd like to keep this as a followup bug.

I wouldn't be surprised if that exposes some issues.  Please file a followup for exposing the barriers only in the appropriate process.

Please do rewrite the comments to be a little more explicit, though.
https://reviewboard.mozilla.org/r/7485/#review6381

> I wouldn't be surprised if that exposes some issues.  Please file a followup for exposing the barriers only in the appropriate process.
> 
> Please do rewrite the comments to be a little more explicit, though.

Filed as bug 1158156.
Attachment #8596482 - Flags: review?(nfroyd) → review+
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

https://reviewboard.mozilla.org/r/7481/#review6385

Ship It!
/me learns to use RB properly
https://reviewboard.mozilla.org/r/7483/#review6527

So, I like the cpp part, but not that much the js part, I think you added too much complication/layers to the simple connection getter, and the executeBeforeShutdown API is also too verbose.
Also, if it's easy, I'd be very happy if you could remove all unneeded changes from the patch, from pointless Task conversions to trailing whitespaces. The smaller the patch, the quicker I can evaluate it... This is scary enough by itself even without hundreds of unneeded changes...

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:95
(Diff revision 1)
> +  print("Check cache");

please use do_print() in xpcshell

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:99
(Diff revision 1)
> +  print("Shutdown the download manager");

ditto

::: toolkit/components/places/Bookmarks.jsm:201
(Diff revision 1)
> +          progress = "fetchBookmarksByURL() resolved";

I don't like that we must complicate the code just to report progress... the previous for let of was largely more readable. I care more about code readability and maintainability than this progress reporting, sorry.

::: toolkit/components/places/Bookmarks.jsm:167
(Diff revision 1)
> +      task: Task.async(function*() {

so, how can we obtain the same with the minimum number of changes needed to Bookmarks.jsm?

I don't like that this executeBeforeShutdown takes a so complex object, it should just take a generator and eventually an additional info argument.

Or even better, can we make it report the last executed query as progress, maybe by wrapping the Sqlite handler with a Proxy.

What I'd like is 
db.executeBeforeShutdown(function* () {}, descriptor);
and have it barely replace Task.spawn where needed.

::: toolkit/components/places/Bookmarks.jsm:478
(Diff revision 1)
> +          let entries = yield fetchBookmarksByURL(item);

:*

::: toolkit/components/places/Bookmarks.jsm:514
(Diff revision 1)
> +        let db = yield Db.promiseDb;

ugh, why can't we just have a single db through a Proxy?
db = yield Db.promiseDB sounds very bad.

::: toolkit/components/places/Database.h:82
(Diff revision 1)
> +  GetConnectionShutdown();

oneline this since we are in a header file

::: toolkit/components/places/Database.cpp:67
(Diff revision 1)
> +// Simulate profile-chang-change. This topic may only be used by

typo profile-chang-change

::: toolkit/components/places/Database.cpp:385
(Diff revision 1)
> +  DebugOnly<nsresult> rv = service->MakeBarrier(

if (service) ...

::: toolkit/components/places/Database.cpp:396
(Diff revision 1)
> +  DebugOnly<nsresult> rv = mBarrier->GetClient(getter_AddRefs(client));

if (mBarrier) ...

::: toolkit/components/places/Database.cpp:425
(Diff revision 1)
> +  }

I think all of these should become oneline
if (NS_WARN_IF(NS_FAILED(rv)) { return rv }

yes, this is basically the same as NS_ENSURE_SUCCESS, but since everyone wants to deprecate that rather than figuring a better name for it, we must go the verbose way.

Since all of these are completely unexpected, warning is the right thing to do.

::: toolkit/components/places/Database.cpp:490
(Diff revision 1)
> +  DebugOnly<nsresult> rv = mBarrier->Wait(this); // FIXME: Double shutdown!

if (mBarrier) ...

::: toolkit/components/places/Database.cpp:620
(Diff revision 1)
> +  if (XRE_GetProcessType() == GeckoProcessType_Content) {

Today I should be able to land the content process part and we could avoid this code.

::: toolkit/components/places/PlacesUtils.jsm:1341
(Diff revision 1)
> -  promiseWrappedConnection: () => gAsyncDBWrapperPromised,
> +  getWrappedConnection: (name) => {

So I think what's I'd want more is a simpler to use API, I don't like much the fact this returns an object that has inside the connection promise...
I'd love if it would return a Proxy that exposes the connection directly and just adds some additional utils onto it.
Something a little bit more transparent to the consumers.

::: toolkit/components/places/PlacesUtils.jsm:2351
(Diff revision 1)
> +XPCOMUtils.defineLazyGetter(this, "gAsyncRWDBConnPromised", () => {

what is this for? leftover?

::: toolkit/components/places/tests/PlacesTestUtils.jsm:37
(Diff revision 1)
> -    return new Promise((resolve, reject) => {
> +    let promise = new Promise((resolve, reject) => {

why the change here? it doesn't seem needed.

So, I
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

https://reviewboard.mozilla.org/r/7481/#review6537
Attachment #8596482 - Flags: review?(mak77)
Sorry, the RB interface is terrible, I was just trying to cancel the request...
note, I added a workaround in bug 1152333 to ensure we .close the Sqlite wrapper when the Places connection goes away. That will shut up the shutdown warnings until we have proper async shutdown support. Should likely be removed here.
https://reviewboard.mozilla.org/r/7483/#review6823

::: toolkit/components/places/Bookmarks.jsm:167
(Diff revision 1)
> +      task: Task.async(function*() {

> so, how can we obtain the same with the minimum number of changes needed to Bookmarks.jsm?

Well, the changes are basically trivial. If we remove the status (which I believe would be a shame), each change is basically a one-liner.

I'm not a huge fan of the idea of adding yet another Proxy to the mix, as I find that Proxies make code harder to understand and debug, even when it's easier to read.

If we decide to go that way, we can probably obtain something along the lines of:

db.executeBeforeShutdown(Task.async(function*(db /*proxy*/) {
  // everything done through the db proxy is logged for AsyncShutdown
}));

Note that I kind of insist on explicitly writing `Task.async`, because I find that promoting a generator to a task without calling `Task` is unhealthy and error-prone.
https://reviewboard.mozilla.org/r/7483/#review6821

> ugh, why can't we just have a single db through a Proxy?
> db = yield Db.promiseDB sounds very bad.

> ugh, why can't we just have a single db through a Proxy?

Sorry, I don't understand what you suggest.

> db = yield Db.promiseDB sounds very bad.

Would you prefer `conn = yield Db.promiseConn`?

> So I think what's I'd want more is a simpler to use API, I don't like much the fact this returns an object that has inside the connection promise...
> I'd love if it would return a Proxy that exposes the connection directly and just adds some additional utils onto it.
> Something a little bit more transparent to the consumers.

I'll see if I manage to come up with something.
https://reviewboard.mozilla.org/r/7483/#review6857

> > ugh, why can't we just have a single db through a Proxy?
> 
> Sorry, I don't understand what you suggest.
> 
> > db = yield Db.promiseDB sounds very bad.
> 
> Would you prefer `conn = yield Db.promiseConn`?

No, I would prefer if what we yield would also be the database.

db = yield PlacesUtils.giveMeADb rather than db.yield PlacesUtils.giveMeADb.ReallyGiveMeIt that's why I was suggesting a Proxy, or you could inject the beforeShutdown util into the db object.
https://reviewboard.mozilla.org/r/7481/#review6897

As discussed overIRC, I have tried to move most of the burden to Sqlite.jsm. Attaching an advanced prototype.
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 79322ae3e8308b4b9e852be81b02b97ac6e0176e https://reviewboard-hg.mozilla.org/gecko/
Attachment #8596482 - Flags: review+ → review?(mak77)
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

https://reviewboard.mozilla.org/r/7481/#review7639

Ship It!

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:95
(Diff revision 2)
> +  print("Check cache");

please use do_print

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:99
(Diff revision 2)
> +  print("Shutdown the download manager");

do_print

::: browser/components/places/tests/unit/test_clearHistory_shutdown.js:97
(Diff revision 2)
> +  let promise = checkCache(FTP_URL);

better name than a generic "promise"? Something like promiseCacheCheck

::: toolkit/components/places/Bookmarks.jsm
(Diff revision 2)
> -

Please restore the removed newline here

::: toolkit/components/places/Database.h:82
(Diff revision 2)
> +  GetConnectionShutdown();

please oneline this

::: toolkit/components/places/Database.h:162
(Diff revision 2)
> -  GetStatement(const nsACString& aQuery) const
> +  GetStatement(const nsACString& aQuery) const;

oneline this too

::: toolkit/components/places/Database.h:191
(Diff revision 2)
> -  GetAsyncStatement(const nsACString& aQuery) const
> +  GetAsyncStatement(const nsACString& aQuery) const;

and also this one

::: toolkit/components/places/Database.h:200
(Diff revision 2)
> +  bool IsShutdownStarted() const;

IsShuttingDown()

::: toolkit/components/places/Database.h:305
(Diff revision 2)
> +  nsRefPtr<class DatabaseShutdown> mConnectionShutdown;

I think you can avoid specifying class here... It might need a forward declaration

::: toolkit/components/places/Database.cpp:31
(Diff revision 2)
> +#include "nsXULAppAPI.h" // For XRE_GetProcessType

should already be there

::: toolkit/components/places/Database.cpp:67
(Diff revision 2)
> +// Simulate profile-chang-change. This topic may only be used by

typo: profile-chang-change...

::: toolkit/components/places/Database.cpp:73
(Diff revision 2)
> +#define TOPIC_SIMULATE_PLACES_MUST_CLOSE_2 "test-simulate-places-must-close-2"

I'd prefer if these had better names than "1 and 2" that point out what they actually do... even test-simulate-places-shutdown-phase-1 would be slightly better.

::: toolkit/components/places/Database.cpp:326
(Diff revision 2)
> +    return mIsStarted;

wouldn't be enough to check state != NOT_STARTED?
So we could probably avoid mIsStarted

::: toolkit/components/places/Database.cpp:385
(Diff revision 2)
> +  DebugOnly<nsresult> rv = service->MakeBarrier(

if (service)

::: toolkit/components/places/Database.cpp:382
(Diff revision 2)
> +  nsCOMPtr<nsIAsyncShutdownService> service = services::GetAsyncShutdown();

s/service/asyncShutdownSvc

::: toolkit/components/places/Database.cpp:396
(Diff revision 2)
> +  DebugOnly<nsresult> rv = mBarrier->GetClient(getter_AddRefs(client));

if (mBarrier) ...

::: toolkit/components/places/Database.cpp:423
(Diff revision 2)
> +  if (NS_FAILED(rv)) {

please oneline

if (NS_WARN_IF(NS_FAILED(rv)) return rv;

everywhere you do

if (NS_FAILED(rv))
  return rv;

unless it's an expected failure (but I don't think you have any)

::: toolkit/components/places/Database.cpp:490
(Diff revision 2)
> +  DebugOnly<nsresult> rv = mBarrier->Wait(this); // FIXME: Double shutdown!

if (!mBarrier) return SOMEERROR

::: toolkit/components/places/Database.cpp:510
(Diff revision 2)
> +  if (os) {

MOZ_ASSERT(os)

::: toolkit/components/places/Database.cpp:545
(Diff revision 2)
> +    if (NS_FAILED(rv)) {

oneline with warning

::: toolkit/components/places/Database.cpp:560
(Diff revision 2)
> +  nsCOMPtr<nsIAsyncShutdownBarrier> barrier = mBarrier.forget();

if (!mBarrier) return SOMEERROR

::: toolkit/components/places/Database.cpp:605
(Diff revision 2)
> +  DebugOnly<nsresult> rv = shutdownPhase->AddBlocker(

if (shutdownPhase)

::: toolkit/components/places/Database.cpp:399
(Diff revision 2)
> +  return client ? client.forget() : nullptr;

hm, probably it's safe to call forget on a null COMPtr... I might have been too overzealous.

::: toolkit/components/places/Database.cpp:616
(Diff revision 2)
> +  nsCOMPtr<nsIAsyncShutdownService> service = services::GetAsyncShutdown();

s/service/asyncShutdownSvc/

::: toolkit/components/places/Database.cpp:2004
(Diff revision 2)
> -  if (strcmp(aTopic, TOPIC_PROFILE_CHANGE_TEARDOWN) == 0) {
> +      || strcmp(aTopic, TOPIC_SIMULATE_PLACES_MUST_CLOSE_1) == 0) {

please move || at the end of the previous line, per coding style

::: toolkit/components/places/Database.cpp:2042
(Diff revision 2)
> -      (void)os->NotifyObservers(nullptr, TOPIC_PLACES_WILL_CLOSE_CONNECTION, nullptr);
> +    shutdownPhase->RemoveBlocker(mConnectionShutdown.get());

if (shutdownPhase)

::: toolkit/components/places/PlacesUtils.jsm:1361
(Diff revision 2)
> +    if (typeof name == "undefined") {

I'd say to just check if (!name), to make it mandatory.

::: toolkit/components/places/PlacesUtils.jsm:2343
(Diff revision 2)
> +    dump(`getItemId: returning ${Object.keys(itemId)}\n`);

debug leftover

::: toolkit/components/places/nsNavHistory.cpp:3102
(Diff revision 2)
> +      ) {

nit: just stick parentheses on the last condition

::: toolkit/components/places/nsPIPlacesDatabase.idl:47
(Diff revision 2)
> +  readonly attribute nsIAsyncShutdownClient shutdownClient;

a minimal javadoc would be nice, very minimal since it's just for internal use.

::: toolkit/components/places/tests/PlacesTestUtils.jsm:37
(Diff revision 2)
> -    return new Promise((resolve, reject) => {
> +    let promise = new Promise((resolve, reject) => {

this looks like an unrelated change, and not even that useful...

::: toolkit/components/places/tests/head_common.js:403
(Diff revision 2)
> +  return promise;

what about just return promise.then(...) ?

::: toolkit/components/places/tests/unit/test_history_notifications.js:67
(Diff revision 2)
> +  yield shutdownPlaces();

why is this now needed?

::: toolkit/modules/Sqlite.jsm:282
(Diff revision 2)
> +   * To avoid this risk, clients are encouraged to use `executeByShutdown` for

s/executeByShutdown/executeBeforeShutdown/

::: toolkit/modules/Sqlite.jsm:302
(Diff revision 2)
> +    if (typeof name != "string") {

just if (!name)

::: toolkit/modules/Sqlite.jsm:316
(Diff revision 2)
> +      // "<closing>".

please wrap around 80 chars

::: toolkit/modules/Sqlite.jsm:320
(Diff revision 2)
> +      // yet.

ditto for wrapping

::: toolkit/modules/Sqlite.jsm:359
(Diff revision 2)
> +          }          

trailing spaces

::: toolkit/modules/Sqlite.jsm:369
(Diff revision 2)
> +    let promiseComplete = promiseResult.catch(() => {});

will this swallow unhandled rejection notifications from the wrapped tasks?
Attachment #8596482 - Flags: review?(mak77) → review+
https://reviewboard.mozilla.org/r/7481/#review7819

> wouldn't be enough to check state != NOT_STARTED?
> So we could probably avoid mIsStarted

I wondered about that, then I decided that `mState` is only for logging purposes. Unless you insist, I'd rather keep it as it is.

> IsShuttingDown()

Not really. This method returns `true` even if shutdown is complete, while a method called `IsShuttingDown` would return `false` if shutdown is complete. Unless you insist, I'd rather keep it as it is.

> if (service)

Done, but I'm still not a big fan of delaying errors until they are harder to catch.

> what about just return promise.then(...) ?

Sure. That was just for uniformity.

> this looks like an unrelated change, and not even that useful...

If my memory serves correctly, it gives error stacks that I could actually use instead of useless error stacks. I can revert this change if you want, your call.

> will this swallow unhandled rejection notifications from the wrapped tasks?

The `return (yield promiseResult)` below ensures that they should still count as uncaught errors.

> why is this now needed?

Well, you can't create two instances of the service at the same time.
I don't remember why this worked before.
Attachment #8596482 - Flags: review+ → review?(mak77)
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r fad63c9f3d442b21e40cd5931537406d60d3e4d8 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r fad63c9f3d442b21e40cd5931537406d60d3e4d8 https://reviewboard-hg.mozilla.org/gecko/
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #32)
> https://reviewboard.mozilla.org/r/7481/#review7819
> 
> > wouldn't be enough to check state != NOT_STARTED?
> > So we could probably avoid mIsStarted
> 
> I wondered about that, then I decided that `mState` is only for logging
> purposes. Unless you insist, I'd rather keep it as it is.

ok

> > IsShuttingDown()
> 
> Not really. This method returns `true` even if shutdown is complete, while a
> method called `IsShuttingDown` would return `false` if shutdown is complete.
> Unless you insist, I'd rather keep it as it is.

as you wish. note we use all around methods called isShuttingDown that keep returning false when shutdown is complete. I'm not sure what would be the usefulness of a method that says "we are not shutting down, but I don't know if we shutdown already or not" :) Probably the best thing would be a tristate bool, but whatever.

> > this looks like an unrelated change, and not even that useful...
> 
> If my memory serves correctly, it gives error stacks that I could actually
> use instead of useless error stacks. I can revert this change if you want,
> your call.

yes please revert it, it looks fancy. If there's an error swallowed somewhere we should not try to workaround it with fancier syntax. Would be nicer to figure why it is.

> > why is this now needed?
> 
> Well, you can't create two instances of the service at the same time.
> I don't remember why this worked before.

But the scope of the test is exactly to check you can't create 2 instances. indeed we check that it throws.
https://reviewboard.mozilla.org/r/7481/#review7903

> Well, you can't create two instances of the service at the same time.
> I don't remember why this worked before.

Ok, I think I got it. The service that was opened improperly was still shutdown asynchronously, which caused an assertion failure because the other service had been opened by then.
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 3b29f275e51bf9655106dc3ac59b811c0d57c0ef https://reviewboard-hg.mozilla.org/gecko/
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #36)
> https://reviewboard.mozilla.org/r/7481/#review7903
> 
> > Well, you can't create two instances of the service at the same time.
> > I don't remember why this worked before.
> 
> Ok, I think I got it. The service that was opened improperly was still
> shutdown asynchronously, which caused an assertion failure because the other
> service had been opened by then.

I see, just document that and we should be fine. it's not something that should happen in real code fwiw.
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 9cb61cebb9191eb85da7a238325edab293781e4c https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 9cb61cebb9191eb85da7a238325edab293781e4c https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 4cef68ac3cbd25873378851b8ea8670167a42dfc https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 6e1f064c4bb2e4626e70eaf77230a4a8d4dd1319 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r bbcc639c002fb2645f2ea9f49b36f70a71682623 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 414fb20acd29797beeb7efd7c3d518abfa498c46 https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r e3dff01d0687f808c607ad605b28eec2f6f55a2e https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 2718c0fa54c69aa63a2963f61914157ef9342e1b https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 198f9f78f1c33cdea53c973a91115dc768d460ca https://reviewboard-hg.mozilla.org/gecko/
you changed clearFile to promiseClearFile, but then called clearFile from inside it. This only reproduces on Windows due to its long file locks. Other OS likely don't hit this code point.
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r aed0c91c4fb1ad4294b8a102536746c8f0e0b11d https://reviewboard-hg.mozilla.org/gecko/
Sorry about that and thanks for the catch.
Sorry, have to cancel the checkin-needed request after coordinating with Marco. Will elaborate in a second on the issue I found.
Status: NEW → ASSIGNED
Keywords: checkin-needed
Was running yesterday's inbound with this patch and noticed SQLite.jsm failures in a debug build:

[83195] WARNING: The SQL statement 'BEGIN undefined TRANSACTION' could not be compiled due to an error: near "undefined": syntax error: file /Users/tim/workspace/mozilla-central/storage/mozStorageConnection.cpp, line 1104

The stack trace for that is:

ConnectionData.prototype<.executeTransaction@resource://gre/modules/Sqlite.jsm:539:29
insertBookmark/<@resource://gre/modules/Bookmarks.jsm:777:11

It looks like somehow the OpenedConnection wrapper is bypassed and "executeTransaction()" is directly called on the ConnectionData object. That should probably be a problem with PlacesUtils.withConnectionWrapper().

It's easy to reproduce by just running "mach mochitest" and it should be visible very early on startup.
more generally we should not bypass OpenedConnection
Comment on attachment 8596482 [details]
MozReview Request: bz://1043863/Yoric

/r/7483 - Bug 1043863 - Using AsyncShutdown to shutdown Places;r=mak
/r/7485 - Bug 1043863 - OpenedConnection.prototype.executeBeforeShutdown;r=mak

Pull down these commits:

hg pull -r 63191f2283044a2fe45b4edb31580b903250bd32 https://reviewboard-hg.mozilla.org/gecko/
https://hg.mozilla.org/mozilla-central/rev/ffde3b92c081
https://hg.mozilla.org/mozilla-central/rev/668d0e7d36e7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox41: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Blocks: 1089695
\o/

Thanks to everyone involved.
Bug 1043863 - AsyncShutdown for e10s;r=froydnj
Attachment #8613768 - Flags: review?(nfroyd)
Bug 1089695 - Async sanitize.js;r=mak
Attachment #8613769 - Flags: review?(mak77)
Comment on attachment 8613768 [details]
MozReview Request: Bug 1043863 - AsyncShutdown for e10s;r=froydnj

Bug 1043863 - AsyncShutdown for e10s;r=froydnj
Attachment #8613768 - Attachment is obsolete: true
Attachment #8613769 - Attachment is obsolete: true
Attachment #8613769 - Flags: review?(mak77)
Depends on: 1172278
No longer depends on: 1172278

        Attachment #8596482 -
        Attachment is obsolete: true
Depends on: 1170719

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: