Closed Bug 1422327 Opened 2 years ago Closed 2 years ago

Intermittent extensions/cookie/test/browser_test_favicon.js | application crashed [@ mozilla::storage::Service::unregisterConnection]

Categories

(Toolkit :: Storage, defect, P3, critical)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- wontfix
firefox60 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: asuth)

References

Details

(Keywords: crash, intermittent-failure)

Crash Data

Attachments

(1 file)

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
Blocks: 1413501
Priority: P5 → P3
Duplicate of this bug: 1436587
Duplicate of this bug: 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.
Duplicate of this bug: 1412092
Duplicate of this bug: 1441013
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 bugmail@asutherland.org:
https://hg.mozilla.org/integration/autoland/rev/fb544e206108
Clean up storage::Connection::Release. r=mak
https://hg.mozilla.org/mozilla-central/rev/fb544e206108
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.