Closed Bug 1395017 Opened 2 years ago Closed 2 years ago

nsContentUtils::CombineResourcePrincipals() is not callable when MediaCacheStream::NotifyDataReceived() runs off the main thread

Categories

(Core :: Audio/Video: Playback, enhancement, P3)

enhancement

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: nobody → jwwang
Blocks: 1382978
Priority: -- → P3
Attachment #8904097 - Flags: review?(cpearce)
Attachment #8904098 - Flags: review?(cpearce)
Attachment #8904099 - Flags: review?(cpearce)
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 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 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+
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.
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
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 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.
Attachment #8904099 - Attachment is obsolete: true
Attachment #8904893 - Attachment is obsolete: true
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)
Attachment #8904097 - Flags: review?(jwwang)
Attachment #8905588 - Flags: review?(cpearce)
Attachment #8904097 - Flags: review+ → review?(cpearce)
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)
Attachment #8904097 - Flags: review?(jwwang)
Attachment #8904097 - Flags: review?(cpearce)
Attachment #8904097 - Flags: review?(cpearce)
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)
Attachment #8904097 - Flags: review?(jwwang)
Attachment #8904097 - Flags: review?(cpearce)
Attachment #8904097 - Flags: review?(cpearce)
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 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)
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.
This issue will be fixed in bug 1401460.
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1401460
Blocks: 1428688
No longer blocks: 1428688
You need to log in before you can comment on or make changes to this bug.