Closed
Bug 1395017
Opened 7 years ago
Closed 7 years ago
nsContentUtils::CombineResourcePrincipals() is not callable when MediaCacheStream::NotifyDataReceived() runs off the main thread
Categories
(Core :: Audio/Video: Playback, enhancement, P3)
Core
Audio/Video: Playback
Tracking
()
RESOLVED
DUPLICATE
of bug 1401460
People
(Reporter: jwwang, Assigned: jwwang)
References
Details
Attachments
(3 files, 2 obsolete files)
http://searchfox.org/mozilla-central/rev/51b3d67a5ec1758bd2fe7d7b6e75ad6b6b5da223/dom/media/MediaCache.cpp#1897 When OMT data delivery is enabled, MediaCacheStream::NotifyDataReceived() will run off the main thread and we can't call MediaCacheStream::UpdatePrincipal(). We need to dispatch a task to the main thread to update the principal.
Assignee | ||
Updated•7 years ago
|
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(cpearce)
Attachment #8904098 -
Flags: review?(cpearce)
Attachment #8904099 -
Flags: review?(cpearce)
Comment 4•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review181572
Attachment #8904097 -
Flags: review?(cpearce) → review+
Comment 5•7 years ago
|
||
mozreview-review |
Comment on attachment 8904098 [details] Bug 1395017. P1 - assert ResourceStreamIterator should either run in the main thread or with the lock held. https://reviewboard.mozilla.org/r/175862/#review181574 ::: dom/media/MediaCache.cpp:239 (Diff revision 1) > , mResourceID(aResourceID) > , mNext(0) > { > + MOZ_ASSERT( > + NS_IsMainThread() || > + (aMediaCache->GetReentrantMonitor().AssertCurrentThreadIn(), true)); I don't understand why you have ", true" in the second clause of the if statement here. Can you explain, or remove it please?
Attachment #8904098 -
Flags: review?(cpearce) → review+
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8904099 [details] Bug 1395017. P3 - always dispatch a task to run UpdatePrincipal() even when CacheClientUpdatePrincipal() already runs in the main thread. https://reviewboard.mozilla.org/r/175864/#review181580
Attachment #8904099 -
Flags: review?(cpearce) → review+
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904098 [details] Bug 1395017. P1 - assert ResourceStreamIterator should either run in the main thread or with the lock held. https://reviewboard.mozilla.org/r/175862/#review181574 > I don't understand why you have ", true" in the second clause of the if statement here. > > Can you explain, or remove it please? You can't convert ReentrantMonitor::AssertCurrentThreadIn() to a bool. So you need a trick like (AssertCurrentThreadIn(), true) to work around it.
Assignee | ||
Comment 8•7 years ago
|
||
Thanks for the review!
Pushed by jwwang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/78625f947f69 P1 - dispatch a task to the main thread to update the principal when necessary. r=cpearce https://hg.mozilla.org/integration/autoland/rev/243ef641e6c3 P2 - assert ResourceStreamIterator should either run in the main thread or with the lock held. r=cpearce https://hg.mozilla.org/integration/autoland/rev/438657f4637d P3 - always dispatch a task to run UpdatePrincipal() even when CacheClientUpdatePrincipal() already runs in the main thread. r=cpearce
Comment 10•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/autoland/rev/0ce1edf053dc for frequent intermittent failures like https://treeherder.mozilla.org/logviewer.html#?job_id=128775996&repo=autoland - since the most common platforms seem to be things like Android and Linux32 debug, my first bet would be a race that you lose more often on slow platforms.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review182138 ::: dom/media/MediaResource.cpp:882 (Diff revision 2) > { > - NS_ASSERTION(NS_IsMainThread(), "Don't call on non-main thread"); > - > + MOZ_ASSERT(NS_IsMainThread()); > + nsCOMPtr<nsIPrincipal> principal; > + nsIScriptSecurityManager* secMan = nsContentUtils::GetSecurityManager(); > + if (secMan && mChannel) { > + secMan->GetChannelResultPrincipal(mChannel, getter_AddRefs(principal)); This line is wrong. GetChannelResultPrincipal() should be passed the mChannel of the resource which is receiving data. When UpdatePrincipal() is called on a cloned resource which has null mChannel, the principal won't be updated correctly. I will rewrite the patches.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904099 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8904893 -
Attachment is obsolete: true
Assignee | ||
Comment 19•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review182386
Attachment #8904097 -
Flags: review?(jwwang)
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(jwwang)
Assignee | ||
Updated•7 years ago
|
Attachment #8905588 -
Flags: review?(cpearce)
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review+ → review?(cpearce)
Assignee | ||
Comment 20•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review182390
Attachment #8904097 -
Flags: review?(jwwang)
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(jwwang)
Attachment #8904097 -
Flags: review?(cpearce)
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(cpearce)
Assignee | ||
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review182396
Attachment #8904097 -
Flags: review?(jwwang)
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(jwwang)
Attachment #8904097 -
Flags: review?(cpearce)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8904097 -
Flags: review?(cpearce)
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review183060 ::: dom/media/MediaResource.cpp:503 (Diff revision 4) > + mCallback->AbstractMainThread()->Dispatch(r.forget()); > > uint32_t count = aCount; > while (count > 0) { > uint32_t read; > - nsresult rv = aStream->ReadSegments(CopySegmentToCache, &closure, count, > + nsresult rv = aStream->ReadSegments(CopySegmentToCache, this, count, &read); You're updating the principal potentially on the main thread *after* you've copied the segments here right? Wouldn't this mean that if the principal has changed, you could end up exposing data that should not be visible? It seems like we need to wait for the principal to be finished updating before we can copy the segment to cache?
Attachment #8904097 -
Flags: review?(cpearce) → review-
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8905588 [details] Bug 1395017. P3 - always dispatch a task to run UpdatePrincipal(). https://reviewboard.mozilla.org/r/177392/#review183062 I'm going to clear the review request here, as I r-'d the previous patch.
Attachment #8905588 -
Flags: review?(cpearce)
Assignee | ||
Comment 26•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8904097 [details] Bug 1395017. P2 - dispatch a task to the main thread to update the principal when necessary. https://reviewboard.mozilla.org/r/175860/#review183060 > You're updating the principal potentially on the main thread *after* you've copied the segments here right? Wouldn't this mean that if the principal has changed, you could end up exposing data that should not be visible? > > It seems like we need to wait for the principal to be finished updating before we can copy the segment to cache? That is a sad truth explained in P3. "When ChannelMediaResource::OnDataAvailable() runs off the main thread, there is no guarantee that the principal will be updated before the new data is observable to the consumer because the principal can only be updated on the main thread while the consumer can access the data off the main thread." If we want to enforce the ordering, we will have to use some kind of synchronization and buffer copying which I think will outweigh the benefit of OMT data delivery.
Assignee | ||
Comment 27•7 years ago
|
||
This issue will be fixed in bug 1401460.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
You need to log in
before you can comment on or make changes to this bug.
Description
•