1623083 - Stack overflow crash in [@ nsTArray_base<T>::SwapArrayElements<T> | mozilla::CycleCollectedJSContext::CleanupIDBTransactions]

Status: RESOLVED FIXED

(Core :: DOM: File, defect, P2)

Description

[Andrew McCreight [:mccr8]]

This bug is for crash report bp-c7138472-dd1a-4bb6-8b11-7c7cc0200317.

Top 10 frames of crashing thread:

0 xul.dll nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>::SwapArrayElements<nsTArrayInfallibleAllocator, nsTArrayInfallibleAllocator> xpcom/ds/nsTArray-inl.h:385
1 xul.dll mozilla::CycleCollectedJSContext::CleanupIDBTransactions xpcom/base/CycleCollectedJSContext.cpp:417
2 xul.dll mozilla::CycleCollectedJSContext::AfterProcessMicrotasks xpcom/base/CycleCollectedJSContext.cpp:482
3 xul.dll mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint xpcom/base/CycleCollectedJSContext.cpp:585
4 xul.dll mozilla::CycleCollectedJSContext::AfterProcessTask xpcom/base/CycleCollectedJSContext.cpp:461
5 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1245
6 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:486
7 xul.dll mozilla::dom::WorkerPrivate::RunCurrentSyncLoop dom/workers/WorkerPrivate.cpp:3820
8 xul.dll mozilla::dom::FileReaderSync::SyncRead dom/file/FileReaderSync.cpp:439
9 xul.dll mozilla::dom::FileReaderSync::SyncRead dom/file/FileReaderSync.cpp:399

While I was looking for variant signatures for the shutdown hang in bug 1622114, I came across this interesting crash. Looking at a few of them, it is a stack overflow on workers, where the apparently unbounded recursion is happening in mozilla::dom::FileReaderSync::SyncRead(nsIInputStream*, char*, unsigned int, unsigned int*). The crash volume looks low, but maybe there's an easy fix.

Comment 1

[Simon Giesecke [:sg] [he/him]]

This looks like a bug in mozilla::dom::FileReaderSync::SyncRead, i.e. in DOM: File, to me. Maybe this can be change not to use recursion at all and clean up its internal resources such as the ReadCallback before the next attempt. In addition, maybe it makes sense to somehow limit the number of retries before giving up?

Comment 2

[Jens Stutte [:jstutte]]

(In reply to Simon Giesecke [:sg] [he/him] from comment #1)

This looks like a bug in mozilla::dom::FileReaderSync::SyncRead, i.e. in DOM: File, to me. Maybe this can be change not to use recursion at all and clean up its internal resources such as the ReadCallback before the next attempt. In addition, maybe it makes sense to somehow limit the number of retries before giving up?

I read here:: "Loop would be nicer, hopefully we don't end up doing too deep recursive calls." as part of the review. Probably it's time to make it a loop without recursion (I see no real need for it, too)?

Comment 3

[Simon Giesecke [:sg] [he/him]]

I think I could rework the function, if someone clarifies if it makes sense to give up at some point, and what a sensible criterion for that might be. With each attempt, we must have read at least read 1 byte, IIUC, so we're definitely making some progress. So maybe replacing the recursion without imposing a limit on retries is sufficient.

baku, can you give me some advice on that?

Comment 4

[Andrea Marchesini [:baku]]

Attachment: Bug 1623083 - No recursion in FileReaderSync::SyncRead, r?sg

Comment 5

[Andrea Marchesini [:baku]]

Yes, we can simply remove the recursion here. Reading the code I changed the code a bit to avoid the recursion. Let me know if it looks correct to you.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d30df7470df1
No recursion in FileReaderSync::SyncRead, r=sg

Comment 7

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/d30df7470df1