can we make MutableBlobStreamListener usefully retargetable?

RESOLVED FIXED in Firefox 60

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: bkelly, Assigned: baku)

Tracking

unspecified
mozilla60
Points:
---

Firefox Tracking Flags

(firefox60 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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)
Posted patch retarget.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #8950555 - Flags: review?(bugs)
Comment on attachment 8950555 [details] [diff] [review]
retarget.patch

Those *Locked() methods are rather error prone.
Please don't rename, but add a aProofOfLock parameter, similar to
https://searchfox.org/mozilla-central/rev/9d47b5fb14152865ad1375eb3ee2e571d81ecdb9/xpcom/threads/LabeledEventQueue.h#47

-// 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-
Fixed comments and aProofOfLock param.
Attachment #8950555 - Attachment is obsolete: true
Attachment #8950568 - Flags: review?(bugs)
Comment on attachment 8950568 [details] [diff] [review]
retarget.patch

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 amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/100460d7a9af
Implement nsIThreadRetargetableStreamListener in MutableBlobStreamListener, r=smaug
I applied the suggested comments before landing.
Flags: needinfo?(amarchesini)
https://hg.mozilla.org/mozilla-central/rev/100460d7a9af
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.