Closed Bug 1461667 Opened 4 years ago Closed 4 years ago

Crash in StructuredCloneCallbacksFreeTransfer


(Core :: DOM: Service Workers, defect, P2)




Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox60 --- unaffected
firefox61 --- unaffected
firefox62 --- fixed


(Reporter: calixte, Assigned: bkelly)


(Blocks 1 open bug)


(Keywords: crash, regression)

Crash Data


(2 files, 1 obsolete file)

This bug was filed from the Socorro interface and is
report bp-ce5afb61-12c2-4e05-94a8-75e6f0180515.

Top 10 frames of crashing thread:

0 StructuredCloneCallbacksFreeTransfer dom/base/StructuredCloneHolder.cpp:123
1 JSStructuredCloneData::discardTransferables [clone .cold.677] 
2 JSAutoStructuredCloneBuffer::clear 
3 mozilla::DefaultDelete<JSAutoStructuredCloneBuffer>::operator const [clone .isra.249] 
4 mozilla::dom::StructuredCloneHolder::~StructuredCloneHolder 
5 SendMessageEventRunnable::~SendMessageEventRunnable dom/serviceworkers/ServiceWorkerPrivate.cpp:523
6 SendMessageEventRunnable::~SendMessageEventRunnable dom/serviceworkers/ServiceWorkerPrivate.cpp:523
7 mozilla::dom::WorkerRunnable::Release dom/workers/WorkerRunnable.cpp:212
8 nsThread::ProcessNextEvent 
9 NS_ProcessPendingEvents 


There are 26 crashes (from 6 installations) in nightly 62 starting with buildid 20180514220126. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1456986.

Flags: needinfo?(bkelly)
Crash Signature: [@ StructuredCloneCallbacksFreeTransfer] → [@ StructuredCloneCallbacksFreeTransfer] [@ @0x0 | JSAutoStructuredCloneBuffer::clear]
Assignee: nobody → bkelly
Flags: needinfo?(bkelly)
Basically the same as bug 1459443.  I guess my changes did not fix it.
I think maybe the StructuredCloneHolder's move constructor is buggy if there are transferrables.  There is a bare closure pointer passed to the JS code that is not updated, AFAICT.
I'm going to change the code to not use the move constructor for now.  If that fixes the crash I'll file a follow-up bug to fix StructuredCloneHolder.
Priority: -- → P2
Comment on attachment 8975823 [details] [diff] [review]
P1 Don't use StructuredCloneHolder's move constructor in ServiceWorkerPrivate. r=baku

Andrea, this reverts some of my previous changes and makes SendMessageEventRunnable extend StructuredCloneHolder again.  The goal here is to avoid using the StructuredCloneHolder move constructor.
Attachment #8975823 - Flags: review?(amarchesini)
Comment on attachment 8975825 [details] [diff] [review]
P2 Remove StructuredCloneHolder's move constructor. r=baku

This removes the StructuredCloneHolder move constructor.  AFAICT this constructor is not safe.  When the holder base creates its mBuffer it passes `this` in as a callback closure.  The move constructor does not update this closure raw pointer at all.  Maybe this can be fixed, but for now lets just remove this footgun.
Attachment #8975825 - Flags: review?(amarchesini)
Attachment #8975823 - Flags: review?(amarchesini) → review+
Comment on attachment 8975825 [details] [diff] [review]
P2 Remove StructuredCloneHolder's move constructor. r=baku

Review of attachment 8975825 [details] [diff] [review]:

Attachment #8975825 - Flags: review?(amarchesini) → review+
Pushed by
P1 Don't use StructuredCloneHolder's move constructor in ServiceWorkerPrivate. r=baku
P2 Remove StructuredCloneHolder's move constructor. r=baku
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
You need to log in before you can comment on or make changes to this bug.