Closed Bug 1422327 Opened 2 years ago Closed 2 years ago
_test _favicon .js | application crashed [@ mozilla::storage::Service::unregister Connection]
59 bytes, text/x-review-board-request
Component: Networking: Cookies → Storage
Product: Core → Toolkit
The signature covers both pre bug 1413501 fixed Thunderbird and Fennec builds, plus some post-fix builds which resemble bug 1417326 in their use of off-the-main-thread storage.
Assignee: nobody → bugmail
Status: NEW → ASSIGNED
See Also: → 1417326
As extra context for review, the crash stacks we were looking at seemed to take the form of: - Crashing at the tail end of minimizeMemory(), where it was dropping the strong refs it had acquired and unregisterConnection() had already dropped the reference so a null deref happened beyond the point where we would have MOZ_ASSERTed, but we weren't a debug build, so we null derefed trying to find the thread to proxy too. This suggests a problem where the Release() method's risky logic was ending up invoking unregisterConnection multiple times. - dom/cache performing a release. I think this is the flip side of the above race where a minimizeMemory was the winner of the unregisterConnection() redundantly-called race. The fix in both cases is the same; make sure we only invoke unregisterConnection once. And to accomplish that, we use an atomic that allows us to make the transition from "alive/registered" to "shutting down/unregistered" exactly once. I had considered this at the tail end of the previous effort, but was trying to minimize churn/etc. Other options considered included: - holding 2 book-keeping references, so that the AsyncCloseConnection ref could just use forget() on that ref when we do the failsafe close. However, that got ugly because getConnections() and its consumers still want strong references, and if you get into them borrowing the references, then we end up needing to do things like have unregisterConnection() do its work on the main thread, etc. - Complicated stuff involving mfbt/ThreadSafeWeakPtr.h and tear-offs and things like that. (nsWeakPtr/nsWeakReference is not thread-safe.) This quickly ends up involving locks and such because weak pointers don't really work for our use-cases. I think the best solution, which this is not, would be to have something logically similar to dom/cache's Connection wrapper class, where the "user" code only ever has references to the wrapper, and it is the wrapper's destructor that initiates the shutdown of the underlying implementation class that can have any number of strong references that get incremented/decremented all the time.
Comment on attachment 8954384 [details] Bug 1422327 - Clean up storage::Connection::Release. https://reviewboard.mozilla.org/r/223470/#review230176 Thank you for the very detailed commenting, it would not be trivial to follow without that.
Attachment #8954384 - Flags: review?(mak77) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/fb544e206108 Clean up storage::Connection::Release. r=mak
You need to log in before you can comment on or make changes to this bug.