Closed Bug 1403023 Opened 2 years ago Closed 2 years ago

Assertion failure: mSentCommitOrAbort, at dom/indexedDB/IDBTransaction.cpp:139

Categories

(Core :: DOM: IndexedDB, defect, P1)

57 Branch
All
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: bc, Assigned: janv)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion)

Attachments

(2 files)

1. https://yandex.ua/search/?lr=142&msid=1505548948.40902.22906.1359&text=%C3%83%C2%83%C3%82%C2%90%C3%83%C2%82%C3%82%C2%BF%C3%83%C2%83%C3%82%C2%90%C3%83%C2%82%C3%82%C2%B5%C3%83%C2%83%C3%82%C2%91%C3%83%C2%82%C3%82%C2%82%C3%83%C2%83%C3%82%C2%91%C3%83%C2%82%C3%82%C2%80%C3%83%C2%83%C3%82%C2%90%C3%83%C2%82%C3%82%C2%BE%C3%83%C2%83%C3%82%C2%91%C3%83%C2%82%C3%82%C2%81%C3%83%C2%83%C3%82%C2%91%C3%83%C2%82%C3%82%C2%8F%C3%83%C2%83%C3%82%C2%90%C3%83%C2%82%C3%82%C2%BD

+ 8 other similar yandex.ua urls.

2. Assertion failure: mSentCommitOrAbort, at z:/build/build/src/dom/indexedDB/IDBTransaction.cpp:139
#01: mozilla::dom::IDBTransaction::`scalar deleting destructor'(unsigned int)
#02: mozilla::DOMEventTargetHelper::DeleteCycleCollectable() [dom/events/DOMEventTargetHelper.cpp:85]
#03: SnowWhiteKiller::~SnowWhiteKiller() [xpcom/base/nsCycleCollector.cpp:2695]
#04: nsCycleCollector::FreeSnowWhite(bool) [xpcom/base/nsCycleCollector.cpp:2892]
#05: nsCycleCollector::BeginCollection(ccType,nsICycleCollectorListener *) [xpcom/base/nsCycleCollector.cpp:3912]
#06: nsCycleCollector::Collect(ccType,js::SliceBudget &,nsICycleCollectorListener *,bool) [xpcom/base/nsCycleCollector.cpp:3761]
#07: nsCycleCollector_collect(nsICycleCollectorListener *) [xpcom/base/nsCycleCollector.cpp:4300]
#08: `anonymous namespace'::WorkerJSRuntime::CustomGCCallback [dom/workers/RuntimeService.cpp:1029]
#09: js::gc::GCRuntime::callGCCallback(JSGCStatus) [js/src/jsgc.cpp:1642]
#10: js::gc::GCRuntime::gcCycle(bool,js::SliceBudget &,JS::gcreason::Reason) [js/src/jsgc.cpp:7157]
#11: js::gc::GCRuntime::collect(bool,js::SliceBudget,JS::gcreason::Reason) [js/src/jsgc.cpp:7289]
#12: js::gc::GCRuntime::gc(JSGCInvocationKind,JS::gcreason::Reason) [js/src/jsgc.cpp:7357]

Beta/57, Nightly/58 Windows 10 only.

s-s due to gc. open if this isn't sensitive.

Once you reproduce you will have to clear all storage for the site before you can reproduce.
Group: core-security → dom-core-security
Can you take a look, Jan?
Flags: needinfo?(jvarga)
I think this is related to bug 1399322. Yeah, I'll take a look.
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Flags: needinfo?(jvarga)
Attached patch patchSplinter Review
Attachment #8912752 - Flags: review?(bugmail)
Hm, I'm actually not sure if this patch fixes it, the stack trace indicates the assertion is hit when cycle collector destroys the object. Maybe this line:
transaction->SetScriptOwner(aDatabase->GetScriptOwner());
keeps the object alive (it's not destrouyed right away in IDBTransaction::Create when HoldWorker fails) and then it's destroyed during cycle collection.

Can someone confirm this ?
Priority: -- → P1
(In reply to Jan Varga [:janv] from comment #4)
> Maybe this line:
> transaction->SetScriptOwner(aDatabase->GetScriptOwner());
> keeps the object alive (it's not destrouyed right away in
> IDBTransaction::Create when HoldWorker fails) and then it's destroyed during
> cycle collection.
> 
> Can someone confirm this ?

I'm still trying to understand the greater context of IDBWrapperCache and IDB's history of sketchy shutdown/GC-handling (understandable as the trailblazer for complicated worker stuff, etc.), but in terms of assertions, I think the situation is reasonably clear:
- Prior to bug 1399322 landing, we would assert that we registered a WorkerHolder.  We'd never get to the destructor in a shutdown situation because we were asserting in Create().
- Now that we can get to the destructor, we hit the assertion which will absolutely fire.

Which is to say, I'm not sure this is actually a security bug, but it wouldn't surprise me if there were some Worker-shutdown-related issues happening.  I'm personally interested in better understanding all of that, but I think we can probably land this fix without security concerns unless we're also seeing non-debug crashes or ASAN errors that relate to this.

In terms of the original question for this situation, I believe if we stopped setting the script owner, then indeed the destructor wouldn't trigger during cycle collection.  However, the destructor would still fire during normal GC.

I made a super-preliminary attempt to see what Yandex was getting up to... it looks like it registers a ServiceWorker for the briefest of durations which makes it hard to look into what it's doing.  In Google Chrome I can at least see that the ServiceWorker seems to try and claim the page before it's an active service worker, which causes an error.  Perhaps that causes the ServiceWorker to freak out and uninstall itself very early in the process, potentially triggering this IndexedDB problem.  I can look into it a little more later, but need to head out for a bit.
Comment on attachment 8912752 [details] [diff] [review]
patch

Review of attachment 8912752 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a sound debug state machine change.

Relatedly, it appears that the IDBTransaction::WorkerHolder::Notify logic may have some logic issues.  It invokes AbortInternal() directly, which doesn't check if the actor is still around, and neither does SendAbort() (which it invokes, and would be the one to set mSentCommitOrAbort), which seems like it could result in a crash.  IDBTransaction::OnRequestFinished() is the place where that guard check normally happens.  I haven't done the full due-diligence on this, however.
Attachment #8912752 - Flags: review?(bugmail) → review+
https://hg.mozilla.org/mozilla-central/rev/946b9c995ec3

I'm just going to land this on Beta since it's debug-only.
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed-beta]
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.