Closed Bug 1437640 Opened 5 years ago Closed 5 years ago

can we make MutableBlobStreamListener usefully retargetable?


(Core :: DOM: File, enhancement)

Not set



Tracking Status
firefox60 --- fixed


(Reporter: bkelly, Assigned: baku)




(1 file, 1 obsolete file)

Over in bug 1437188 Eric noticed we are getting a lot of failed channel retargeting when running a blob-heavy WPT test.  This is because MutableBlobStreamListener  and its backing storage class are not thread safe.

For now we are avoiding the retarget when we know we explicitly add a non-thread-safe listener like this, but could we make it thread-safe?  Could MutableBlobStreamListener be retargeted to another thread usefully?

I say "usefully", because if the implementation is just bouncing the runnables to the main thread, then that its probably just best not to retarget.  It would only really be helpful if it could write the buffers OMT.

Andrea, what do you think?
Flags: needinfo?(amarchesini)
Attached patch retarget.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8950555 - Flags: review?(bugs)
Comment on attachment 8950555 [details] [diff] [review]

Those *Locked() methods are rather error prone.
Please don't rename, but add a aProofOfLock parameter, similar to

-// This class is main-thread only.
+// This class is main-thread only, but ::Append() can be called on any thread.
 class MutableBlobStorage final

Odd comment. "main-thread only" ... but can be used on any thread after all. And Append calls many other methods.
So, the class isn't main-thread only.
Attachment #8950555 - Flags: review?(bugs) → review-
Attached patch retarget.patchSplinter Review
Fixed comments and aProofOfLock param.
Attachment #8950555 - Attachment is obsolete: true
Attachment #8950568 - Flags: review?(bugs)
Comment on attachment 8950568 [details] [diff] [review]

better to push to try (debug builds)
Attachment #8950568 - Flags: review?(bugs) → review+
Andrea, when you land this can you remove the `if (rr && rl)` bit of code that was added in bug 1437188?  We only added that to work around MutableBlobStreamListener being non-retargetable.  The WPT mimesniff/mime-type/parsers.any.html test should trigger this code a lot if you want to verify that the warnings are indeed still gone.
Flags: needinfo?(amarchesini)
Pushed by
Implement nsIThreadRetargetableStreamListener in MutableBlobStreamListener, r=smaug
I applied the suggested comments before landing.
Flags: needinfo?(amarchesini)
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.