Closed Bug 1818576 Opened 3 years ago Closed 2 years ago

Assertion failure: !IsReadableStreamLocked(aSource), at /dom/streams/ReadableStreamPipeTo.cpp:904

Categories

(Core :: DOM: Streams, defect, P3)

Product:
Core
Component:
DOM: Streams
Platform:
x86_64
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
115 Branch
Tracking Flags:
Tracking Status
firefox-esr102 --- wontfix
firefox110 --- wontfix
firefox111 --- wontfix
firefox112 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- verified

People

(Reporter: jkratzer, Assigned: sfink)

References

(Blocks 2 open bugs, Regression)

Details

(Keywords: regression, testcase, Whiteboard: [bugmon:bisected,confirmed], [wptsync upstream])

Attachments

(7 files)

Testcase
724 bytes, text/plain
Details
Bug 1818576 - Double check whether the streams are locked before transfer r=evilpie
48 bytes, text/x-phabricator-request
Details | Review
Bug 1818576 - Test infrastructure for custom serializable objects
48 bytes, text/x-phabricator-request
Details | Review
Bug 1818576 - JSStructuredCloneData should own any transferables immediately from the beginning of writing
48 bytes, text/x-phabricator-request
Details | Review
Bug 1818576 - If the readTransfer() hook returns false, report an error (instead of throwing an uncatchable exception)
48 bytes, text/x-phabricator-request
Details | Review
Bug 1818576 - Report error if a non-transferable is passed in the transferable array
48 bytes, text/x-phabricator-request
Details | Review
Bug 1818576 - Update docs on reportError to reflect how it actually gets called, and how it is used in practice.
48 bytes, text/x-phabricator-request
Details | Review

Testcase found while fuzzing mozilla-central rev 16f49fd3a5dc (built with: --enable-debug --enable-fuzzing).

Testcase can be reproduced using the following commands:

$ pip install fuzzfetch grizzly-framework
$ python -m fuzzfetch --build 16f49fd3a5dc --debug --fuzzing -n firefox
$ python -m grizzly.replay ./firefox/firefox testcase.html

Assertion failure: !IsReadableStreamLocked(aSource), at /dom/streams/ReadableStreamPipeTo.cpp:904

    ==1030348==ERROR: UndefinedBehaviorSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fd3a3cc75f8 bp 0x7fd390d87000 sp 0x7fd390d86f50 T1030513)
    ==1030348==The signal is caused by a WRITE memory access.
    ==1030348==Hint: address points to the zero page.
        #0 0x7fd3a3cc75f8 in mozilla::dom::streams_abstract::ReadableStreamPipeTo(mozilla::dom::ReadableStream*, mozilla::dom::WritableStream*, bool, bool, bool, mozilla::dom::AbortSignal*, mozilla::ErrorResult&) /dom/streams/ReadableStreamPipeTo.cpp:904:3
        #1 0x7fd3a3cd8680 in mozilla::dom::ReadableStream::Transfer(JSContext*, mozilla::dom::UniqueMessagePortId&) /dom/streams/Transferable.cpp:841:7
        #2 0x7fd3a0ab70b9 in mozilla::dom::StructuredCloneHolder::CustomWriteTransferHandler(JSContext*, JS::Handle<JSObject*>, unsigned int*, JS::TransferableOwnership*, void**, unsigned long*) /dom/base/StructuredCloneHolder.cpp:1424:22
        #3 0x7fd3a657ae3b in JSStructuredCloneWriter::transferOwnership() /js/src/vm/StructuredClone.cpp:2316:12
        #4 0x7fd3a656be4f in JSStructuredCloneWriter::write(JS::Handle<JS::Value>) /js/src/vm/StructuredClone.cpp:2449:10
        #5 0x7fd3a656ad23 in WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, JSStructuredCloneData*, JS::StructuredCloneScope, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*, JS::Value const&) /js/src/vm/StructuredClone.cpp:754:10
        #6 0x7fd3a658507a in JS_WriteStructuredClone(JSContext*, JS::Handle<JS::Value>, JSStructuredCloneData*, JS::StructuredCloneScope, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*, JS::Handle<JS::Value>) /js/src/vm/StructuredClone.cpp:3882:10
        #7 0x7fd3a65865ec in JSAutoStructuredCloneBuffer::write(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::CloneDataPolicy const&, JSStructuredCloneCallbacks const*, void*) /js/src/vm/StructuredClone.cpp:4003:13
        #8 0x7fd3a0ab00f2 in mozilla::dom::StructuredCloneHolderBase::Write(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::CloneDataPolicy const&) /dom/base/StructuredCloneHolder.cpp:276:17
        #9 0x7fd3a0ab0a1b in mozilla::dom::StructuredCloneHolder::Write(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::CloneDataPolicy const&, mozilla::ErrorResult&) /dom/base/StructuredCloneHolder.cpp:363:35
        #10 0x7fd3a3c0ec50 in mozilla::dom::WorkerPrivate::PostMessageToParent(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Sequence<JSObject*> const&, mozilla::ErrorResult&) /dom/workers/WorkerPrivate.cpp:4617:13
        #11 0x7fd3a1ce1f1f in mozilla::dom::DedicatedWorkerGlobalScope_Binding::postMessage(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) /builds/worker/workspace/obj-build/dom/bindings/DedicatedWorkerGlobalScopeBinding.cpp:197:32
        #12 0x7fd3a208c93a in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::MaybeGlobalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /dom/bindings/BindingUtils.cpp:3318:13
        #13 0x7fd3a66040c6 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /js/src/vm/Interpreter.cpp:459:13
        #14 0x7fd3a66039ef in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /js/src/vm/Interpreter.cpp:547:12
        #15 0x7fd3a65f562f in CallFromStack /js/src/vm/Interpreter.cpp:619:10
        #16 0x7fd3a65f562f in Interpret(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:3362:16
        #17 0x7fd3a65e8cee in js::RunScript(JSContext*, js::RunState&) /js/src/vm/Interpreter.cpp:431:13
        #18 0x7fd3a66038eb in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /js/src/vm/Interpreter.cpp:579:13
        #19 0x7fd3a6604e1c in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /js/src/vm/Interpreter.cpp:646:8
        #20 0x7fd3a66c12dc in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /js/src/vm/CallAndConstruct.cpp:117:10
        #21 0x7fd3a1d614d1 in mozilla::dom::EventHandlerNonNull::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/EventHandlerBinding.cpp:65:37
        #22 0x7fd3a26c09a9 in void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget>>(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/EventHandlerBinding.h:82:12
        #23 0x7fd3a26bfb96 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*) /dom/events/JSEventHandler.cpp:199:12
        #24 0x7fd3a26a061d in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*) /dom/events/EventListenerManager.cpp:1314:22
        #25 0x7fd3a26a1289 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool) /dom/events/EventListenerManager.cpp:1504:17
        #26 0x7fd3a26960c6 in HandleEvent /dom/events/EventListenerManager.h:395:5
        #27 0x7fd3a26960c6 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /dom/events/EventDispatcher.cpp:347:17
        #28 0x7fd3a26955fb in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /dom/events/EventDispatcher.cpp:549:16
        #29 0x7fd3a2697db5 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /dom/events/EventDispatcher.cpp:1122:11
        #30 0x7fd3a269a996 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*) /dom/events/EventDispatcher.cpp
        #31 0x7fd3a266edfb in mozilla::DOMEventTargetHelper::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) /dom/events/DOMEventTargetHelper.cpp:176:17
        #32 0x7fd3a26a7cf2 in mozilla::dom::EventTarget::DispatchEvent(mozilla::dom::Event&) /dom/events/EventTarget.cpp:180:13
        #33 0x7fd3a3bd1277 in mozilla::dom::MessageEventRunnable::DispatchDOMEvent(JSContext*, mozilla::dom::WorkerPrivate*, mozilla::DOMEventTargetHelper*, bool) /dom/workers/MessageEventRunnable.cpp:104:12
        #34 0x7fd3a3c190de in mozilla::dom::WorkerRunnable::Run() /dom/workers/WorkerRunnable.cpp:377:12
        #35 0x7fd39ee1eaf2 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1219:16
        #36 0x7fd39ee24e7d in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:477:10
        #37 0x7fd3a3c073d4 in mozilla::dom::WorkerPrivate::DoRunLoop(JSContext*) /dom/workers/WorkerPrivate.cpp:3275:7
        #38 0x7fd3a3bee37d in mozilla::dom::workerinternals::(anonymous namespace)::WorkerThreadPrimaryRunnable::Run() /dom/workers/RuntimeService.cpp:2043:42
        #39 0x7fd39ee1eaf2 in nsThread::ProcessNextEvent(bool, bool*) /xpcom/threads/nsThread.cpp:1219:16
        #40 0x7fd39ee24e7d in NS_ProcessNextEvent(nsIThread*, bool) /xpcom/threads/nsThreadUtils.cpp:477:10
        #41 0x7fd39fa73893 in mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) /ipc/glue/MessagePump.cpp:330:5
        #42 0x7fd39f9944d8 in MessageLoop::RunInternal() /ipc/chromium/src/base/message_loop.cc:381:10
        #43 0x7fd39f9943e1 in RunHandler /ipc/chromium/src/base/message_loop.cc:374:3
        #44 0x7fd39f9943e1 in MessageLoop::Run() /ipc/chromium/src/base/message_loop.cc:356:3
        #45 0x7fd39ee19ee7 in nsThread::ThreadFunc(void*) /xpcom/threads/nsThread.cpp:384:10
        #46 0x7fd3b2d11c86 in _pt_root /nsprpub/pr/src/pthreads/ptthread.c:201:5
        #47 0x7fd3b2a94b42 in start_thread nptl/pthread_create.c:442:8
        #48 0x7fd3b2b269ff  misc/../sysdeps/unix/sysv/linux/x86_64/clone3.S:81
    
    UndefinedBehaviorSanitizer can not provide additional info.
    SUMMARY: UndefinedBehaviorSanitizer: SEGV /dom/streams/ReadableStreamPipeTo.cpp:904:3 in mozilla::dom::streams_abstract::ReadableStreamPipeTo(mozilla::dom::ReadableStream*, mozilla::dom::WritableStream*, bool, bool, bool, mozilla::dom::AbortSignal*, mozilla::ErrorResult&)
    ==1030348==ABORTING
Attached file Testcase —

Oh, this is an interesting case. It seems the postMessage from the worker should fail as one of them should be locked by the result of transferring another one, but maybe it's not working that way here?

Assignee: nobody → krosylight

Verified bug as reproducible on mozilla-central 20230223172038-8abe8c3a6233.
The bug appears to have been introduced in the following build range:

Start: aaaed875acb35024eb955fca92ba50ae244be85c (20220519114425)
End: cc776278c4ea98788c42b90a53d1c6c37fdf47e7 (20220519125856)
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aaaed875acb35024eb955fca92ba50ae244be85c&tochange=cc776278c4ea98788c42b90a53d1c6c37fdf47e7

Keywords: regression
Whiteboard: [bugmon:confirm] → [bugmon:bisected,confirmed]

Based on comment #3, this bug contains a bisection range found by bugmon. However, the Regressed by field is still not filled.

:saschanaz, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.

Flags: needinfo?(krosylight)
Flags: needinfo?(krosylight)
Regressed by: 1659025

Set release status flags based on info from the regressing bug 1659025

status-firefox110: --- → affected
status-firefox111: --- → affected
status-firefox112: --- → affected
status-firefox-esr102: --- → affected

Status checks are done in batch before any transfer, but transferring streams can change the status of parent/child of TransformStream and cause an assertion failure.

Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/30075641dc6f Double check whether the streams are locked before transfer r=evilpie
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38710 for changes under testing/web-platform/tests
Whiteboard: [bugmon:bisected,confirmed] → [bugmon:bisected,confirmed], [wptsync upstream]

Backed out for causing leaks.

[task 2023-02-25T14:35:26.943Z] 14:35:26     INFO - TEST-START | /streams/transferable/writable-stream.html
<...>
[task 2023-02-25T14:35:28.447Z] 14:35:28     INFO - TEST-PASS | leakcheck | tab no leaks detected!
[task 2023-02-25T14:35:28.447Z] 14:35:28     INFO - leakcheck | Processing leak log file /var/folders/tf/93bb5jv56vb7tmkplpdbr3_0000014/T/tmpyiazoj0w/runtests_leaks_1049.log
[task 2023-02-25T14:35:28.448Z] 14:35:28     INFO - 
[task 2023-02-25T14:35:28.448Z] 14:35:28     INFO - == BloatView: ALL (cumulative) LEAK AND BLOAT STATISTICS, default process 2526
[task 2023-02-25T14:35:28.448Z] 14:35:28     INFO - 
[task 2023-02-25T14:35:28.449Z] 14:35:28     INFO -      |<----------------Class--------------->|<-----Bytes------>|<----Objects---->|
[task 2023-02-25T14:35:28.449Z] 14:35:28     INFO -      |                                      | Per-Inst   Leaked|   Total      Rem|
[task 2023-02-25T14:35:28.450Z] 14:35:28     INFO -    0 |TOTAL                                 |       47     3096| 3475383       32|
[task 2023-02-25T14:35:28.450Z] 14:35:28     INFO -  212 |CondVar                               |       80       80|     738        1|
[task 2023-02-25T14:35:28.450Z] 14:35:28     INFO -  625 |MessagePortService                    |       56       56|      36        1|
[task 2023-02-25T14:35:28.451Z] 14:35:28     INFO -  626 |MessagePortServiceData                |       64      768|     296       12|
[task 2023-02-25T14:35:28.451Z] 14:35:28     INFO -  642 |Mutex                                 |       96      192|   15569        2|
[task 2023-02-25T14:35:28.452Z] 14:35:28     INFO - 1306 |SharedJSAllocatedData                 |      168     1008|    1648        6|
[task 2023-02-25T14:35:28.453Z] 14:35:28     INFO - 1309 |SharedMessageBody                     |       80      480|     422        6|
[task 2023-02-25T14:35:28.453Z] 14:35:28     INFO - 1401 |ThreadEventTarget                     |       48       48|      72        1|
[task 2023-02-25T14:35:28.454Z] 14:35:28     INFO - 1406 |ThreadTargetSink                      |       16       16|      72        1|
[task 2023-02-25T14:35:28.455Z] 14:35:28     INFO - 2117 |nsStringBuffer                        |        8        8|  175827        1|
[task 2023-02-25T14:35:28.456Z] 14:35:28     INFO - 2158 |nsThread                              |      440      440|      96        1|
[task 2023-02-25T14:35:28.456Z] 14:35:28     INFO - 
[task 2023-02-25T14:35:28.457Z] 14:35:28     INFO - nsTraceRefcnt::DumpStatistics: 2263 entries
[task 2023-02-25T14:35:28.457Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 CondVar
[task 2023-02-25T14:35:28.457Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 MessagePortService
[task 2023-02-25T14:35:28.458Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 12 MessagePortServiceData
[task 2023-02-25T14:35:28.458Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 2 Mutex
[task 2023-02-25T14:35:28.459Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 6 SharedJSAllocatedData
[task 2023-02-25T14:35:28.459Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 6 SharedMessageBody
[task 2023-02-25T14:35:28.459Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 ThreadEventTarget
[task 2023-02-25T14:35:28.460Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 ThreadTargetSink
[task 2023-02-25T14:35:28.460Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 nsStringBuffer
[task 2023-02-25T14:35:28.461Z] 14:35:28     INFO - TEST-INFO | leakcheck | default leaked 1 nsThread
[task 2023-02-25T14:35:28.461Z] 14:35:28     INFO - TEST-UNEXPECTED-FAIL | leakcheck | default 3096 bytes leaked (CondVar, MessagePortService, MessagePortServiceData, Mutex, SharedJSAllocatedData, ...)
[task 2023-02-25T14:35:28.461Z] 14:35:28     INFO - 
[task 2023-02-25T14:35:28.463Z] 14:35:28     INFO - Closing logging queue
[task 2023-02-25T14:35:28.464Z] 14:35:28     INFO - queue closed
Flags: needinfo?(krosylight)
Upstream PR was closed without merging
Flags: needinfo?(krosylight)
See Also: → 1696783
Severity: -- → S3
Flags: needinfo?(krosylight)
Priority: -- → P3
Duplicate of this bug: 1821224

Hmm, I expected StructuredCloneCallbacksFreeTransfer to be called after a partially successful transfer, but it seems it's never called at all? Not sure in which situation it's called.

Flags: needinfo?(krosylight)

Hi Steve, do you have any idea what should be done to solve the leak issue in comment #9?

Situation: Transferring two objects. Transferring the first object succeeded with involving a MessagePort. But transferring the second one fails, and no freeing happens, causing a dangling port.

I see JSStructuredCloneData::discardTransferables is called but it halts on ownTransferables_ != OwnTransferablePolicy::OwnsTransferablesIfAny check and thus no freeing happens.

The relevant call stack:

xul.dll!JSStructuredCloneData::discardTransferables() Line 1038 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:1038)
xul.dll!JSStructuredCloneData::~JSStructuredCloneData() Line 1027 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:1027)
xul.dll!js::SCOutput::~SCOutput() Line 328 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:328)
xul.dll!JSStructuredCloneWriter::~JSStructuredCloneWriter() Line 1137 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:1137)
xul.dll!WriteStructuredClone(JSContext * cx, JS::Handle<JS::Value> v, JSStructuredCloneData * bufp, JS::StructuredCloneScope scope, const JS::CloneDataPolicy & cloneDataPolicy, const JSStructuredCloneCallbacks * cb, void * cbClosure, const JS::Value & transferable) Line 759 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:759)
xul.dll!JS_WriteStructuredClone(JSContext * cx, JS::Handle<JS::Value> value, JSStructuredCloneData * bufp, JS::StructuredCloneScope scope, const JS::CloneDataPolicy & cloneDataPolicy, const JSStructuredCloneCallbacks * optionalCallbacks, void * closure, JS::Handle<JS::Value> transferable) Line 3882 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:3882)
xul.dll!JSAutoStructuredCloneBuffer::write(JSContext * cx, JS::Handle<JS::Value> value, JS::Handle<JS::Value> transferable, const JS::CloneDataPolicy & cloneDataPolicy, const JSStructuredCloneCallbacks * optionalCallbacks, void * closure) Line 4003 (c:\Users\sasch\Documents\GitHub\gecko-dev\js\src\vm\StructuredClone.cpp:4003)
xul.dll!mozilla::dom::StructuredCloneHolderBase::Write(JSContext * aCx, JS::Handle<JS::Value> aValue, JS::Handle<JS::Value> aTransfer, const JS::CloneDataPolicy & aCloneDataPolicy) Line 276 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\base\StructuredCloneHolder.cpp:276)
xul.dll!mozilla::dom::StructuredCloneHolder::Write(JSContext * aCx, JS::Handle<JS::Value> aValue, JS::Handle<JS::Value> aTransfer, const JS::CloneDataPolicy & aCloneDataPolicy, mozilla::ErrorResult & aRv) Line 363 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\base\StructuredCloneHolder.cpp:363)
xul.dll!nsContentUtils::StructuredClone(JSContext * aCx, nsIGlobalObject * aGlobal, JS::Handle<JS::Value> aValue, const mozilla::dom::StructuredSerializeOptions & aOptions, JS::MutableHandle<JS::Value> aRetval, mozilla::ErrorResult & aError) Line 10091 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\base\nsContentUtils.cpp:10091)
xul.dll!nsGlobalWindowInner::StructuredClone(JSContext * aCx, JS::Handle<JS::Value> aValue, const mozilla::dom::StructuredSerializeOptions & aOptions, JS::MutableHandle<JS::Value> aRetval, mozilla::ErrorResult & aError) Line 7640 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\base\nsGlobalWindowInner.cpp:7640)
xul.dll!mozilla::dom::Window_Binding::structuredClone(JSContext * cx_, JS::Handle<JSObject *> obj, void * void_self, const JSJitMethodCallArgs & args) Line 20460 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\dom\bindings\WindowBinding.cpp:20460)

And it seems that flag is set here when the write fails: https://searchfox.org/mozilla-central/rev/dbec4165e4c26a0ff970b614842b689e8357593c/js/src/vm/StructuredClone.cpp#4012

Maybe there should be some way to still try freeing things even if it's failed? What do you think?

Flags: needinfo?(sfink)

Set release status flags based on info from the regressing bug 1659025

status-firefox113: --- → affected

(In reply to Kagami [:saschanaz] from comment #13)

And it seems that flag is set here when the write fails: https://searchfox.org/mozilla-central/rev/dbec4165e4c26a0ff970b614842b689e8357593c/js/src/vm/StructuredClone.cpp#4012

Maybe there should be some way to still try freeing things even if it's failed? What do you think?

What do I think? I think that I have been very slow in responding to this needinfo, and I apologize for that. I think that what you're saying seems exactly correct. I think it's a bad idea to persist with the current clearly broken approach of "if anything goes wrong while transferring, pretend like nothing up to that point has been committed to and we can just drop it all on the floor".

It's a tricky lifecycle thing. (I'm writing stuff out for my own benefit here.) The process for a transferred ArrayBuffer is:

  1. parseTransferable collects the set of transferable objects in the input
  2. writeHeader writes out a basic structured clone header
  3. writeTransferMap writes out SCTAG_TRANSFER_MAP_PENDING_ENTRY with ownership=SCTAG_TMO_UNFILLED, along with empty space to store a pointer later. ("TMO" = "transfer map ownership")
  • at this point, failure should do nothing.
  1. the main write happens. When the transferred ArrayBuffer is encountered, it is replaced with a back reference to an already-existing object (similar to if you were serializing [obj, obj])
  2. transferOwnership is called and "rewinds" to the transfer map from step 3, then loops over the remembered transferables.
  3. a transferable ArrayBuffer updates SCTAG_TRANSFER_MAP_PENDING_ENTRY → SCTAG_TRANSFER_MAP_STORED_ARRAY_BUFFER with ownership=SCTAG_TMO_ALLOC_DATA or SCTAG_TMO_MAPPED_DATA and detaches the original ArrayBuffer.
  • this means that the clone buffer now owns the data. If an error happens after this point, it should free it. But that won't happen with the current code, because the JSStructuredCloneData is not OwnsTransferablesIfAny yet.
  • a custom transferable would have writeTransfer called here.
  • as a special case, a DifferentProcess structured clone would instead set the ownership as SCTAG_TMO_UNOWNED, copy the actual data into the end of the buffer, and detach the source ArrayBuffer. In this case, there's nothing to free; the original data pointer is freed when it is detached, and the clone buffer has a copy of the data. There is no equivalent of this available to custom transferables; freeTransfer will never be called for DifferentProcess clone buffers.
  1. JSStructuredCloneWriter passes ownership of the clone buffer to the caller's JSStructuredCloneData object by calling extractBuffer.
  2. If everything was ok, JSAutoStructuredCloneBuffer::write changes its JSStructuredCloneData, the one that now owns the data including transferables, from NoTransferables → OwnsTransferablesifAny.
    8b. On an error, it sets the ownership to NoTransferables, but that's what it already was so it has no effect.
  • this is one of many reasons JSAutoStructuredCloneBuffer should be taken out and shot. It should not be the one maintaining the data's ownership flag. JSAutoStructuredCloneBuffer doesn't serve any useful purpose, honestly.

I think that the flag should be set very early. Nothing will happen before transferOwnership is called, so this would have no effect on errors during the main writing. If there is an error while transferring some object, then all objects that were already transferred will be discarded, no objects that have not yet been transferred will be discarded, and the one currently being transferred will be discarded if and only if its custom writeTransfer callback has already updated its ownership. Which seems correct.

I have a vague memory that when I set it up like that, I was thinking that anything the embedding was doing (especially with respect to transferring) was the embedding's responsibility to undo once it saw a false return value. Which now seems like quite a stupid way to think about it, since it breaks modularity: you can't just implement a single custom transferable, you have to have a recovery process too.

I mean, I suppose you could do it, by queuing up a "discard closure" every time you transfer anything, and running them all on failure. But that seems more complex than maintaining an invariant on the data that is consistent enough for discardTransferables to work without breaking anything.

I'm going to need to implement a bit of testing functionality to properly test this.

Assignee: krosylight → sphink
Status: NEW → ASSIGNED
Flags: needinfo?(sfink)
Blocks: 1832257
Pushed by sfink@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e51a8f2bf558 Report error if a non-transferable is passed in the transferable array r=spidermonkey-reviewers,mgaudet https://hg.mozilla.org/integration/autoland/rev/ea2856b68cec Update docs on reportError to reflect how it actually gets r=spidermonkey-reviewers,mgaudet https://hg.mozilla.org/integration/autoland/rev/0e22cec33a2f Test infrastructure for custom serializable objects r=spidermonkey-reviewers,mgaudet https://hg.mozilla.org/integration/autoland/rev/b653368f6a7f JSStructuredCloneData should own any transferables immediately from the beginning of writing r=saschanaz,iain https://hg.mozilla.org/integration/autoland/rev/792bae42c188 If the readTransfer() hook returns false, report an error (instead of throwing an uncatchable exception) r=spidermonkey-reviewers,saschanaz,mgaudet

Bug marked as FIXED but still reproduces on mozilla-central 20230511040639-da13ef752e22. If you believe this to be incorrect, please remove the bugmon keyword to prevent further analysis.

Status: RESOLVED → REOPENED
status-firefox115: fixed → affected
Resolution: FIXED → ---

Yeah, I need to land my patch too.

Pushed by krosylight@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff57154426c4 Double check whether the streams are locked before transfer r=evilpie

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

Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox115: affected → fixed
Resolution: --- → FIXED
Upstream PR merged by moz-wptsync-bot

Verified bug as fixed on rev mozilla-central 20230516212859-035b9c71b042.
Removing bugmon keyword as no further action possible. Please review the bug and re-add the keyword for further analysis.

Status: RESOLVED → VERIFIED
status-firefox115: fixed → verified
Keywords: bugmon

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox114 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(sphink)

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #29)

The patch landed in nightly and beta is affected.
:sfink, is this bug important enough to require an uplift?

I don't think I'm the right one to answer that.

Flags: needinfo?(sphink) → needinfo?(krosylight)

This is kinda edge case and thus I'd like to just let it ride the train, especially given that Steve's patches are not trivial ones.

Flags: needinfo?(krosylight)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: