Closed Bug 1428242 Opened 3 years ago Closed 3 years ago

Assert we always take the MediaCache monitor off the main thread

Categories

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

Product:
Core
Component:
Audio/Video: Playback
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: jwwang, Assigned: jwwang)

References

Details

Attachments

(3 files)

Bug 1428242. P1 - assert we always take the MediaCache monitor off the main thread.
59 bytes, text/x-review-board-request
bechen
: review+
gerald
: review+
Details
Bug 1428242. P2 - MediaCache::ReadCacheFile() doesn't need to drop the cache monitor.
59 bytes, text/x-review-board-request
bechen
: review+
gerald
: review+
Details
Bug 1428242. P3 - use a non-reentrant monitor.
59 bytes, text/x-review-board-request
bechen
: review+
gerald
: review+
Details 
We have no more functions to take the lock on the main thread.
Assignee: nobody → jwwang
Blocks: 1369263
Priority: -- → P3
Attachment #8940052 - Flags: review?(bechen)
Attachment #8940053 - Flags: review?(bechen)
Attachment #8940054 - Flags: review?(bechen)
Comment on attachment 8940052 [details]
Bug 1428242. P1 - assert we always take the MediaCache monitor off the main thread.

https://reviewboard.mozilla.org/r/210326/#review216060
Attachment #8940052 - Flags: review?(bechen) → review+
Comment on attachment 8940053 [details]
Bug 1428242. P2 - MediaCache::ReadCacheFile() doesn't need to drop the cache monitor.

https://reviewboard.mozilla.org/r/210328/#review216062
Attachment #8940053 - Flags: review?(bechen) → review+
Comment on attachment 8940054 [details]
Bug 1428242. P3 - use a non-reentrant monitor.

https://reviewboard.mozilla.org/r/210330/#review216064
Attachment #8940054 - Flags: review?(bechen) → review+
Attachment #8940052 - Flags: review?(gsquelart)
Attachment #8940053 - Flags: review?(gsquelart)
Attachment #8940054 - Flags: review?(gsquelart)
Comment on attachment 8940052 [details]
Bug 1428242. P1 - assert we always take the MediaCache monitor off the main thread.

https://reviewboard.mozilla.org/r/210326/#review216068
Attachment #8940052 - Flags: review?(gsquelart) → review+
Comment on attachment 8940053 [details]
Bug 1428242. P2 - MediaCache::ReadCacheFile() doesn't need to drop the cache monitor.

https://reviewboard.mozilla.org/r/210328/#review216070

::: dom/media/MediaCache.cpp:818
(Diff revision 1)
> -  RefPtr<MediaBlockCacheBase> blockCache = mBlockCache;
> +  return mBlockCache->Read(aOffset, reinterpret_cast<uint8_t*>(aData), aLength, aBytes);
> -  if (!blockCache) {
> -    return NS_ERROR_FAILURE;
> -  }

You have removed the null-check of mBlockCache.
It seems reasonable to me, as it is only written once at construction time.
But I think you should at least talk about this change in the commit message, or even do it in a separate patch.
Attachment #8940053 - Flags: review?(gsquelart) → review+
Comment on attachment 8940054 [details]
Bug 1428242. P3 - use a non-reentrant monitor.

https://reviewboard.mozilla.org/r/210330/#review216072

Yes! :-)

Oh, was the unlock-while-doubly-locked the problem with bug 1374173?
Attachment #8940054 - Flags: review?(gsquelart) → review+
Comment on attachment 8940053 [details]
Bug 1428242. P2 - MediaCache::ReadCacheFile() doesn't need to drop the cache monitor.

https://reviewboard.mozilla.org/r/210328/#review216070

> You have removed the null-check of mBlockCache.
> It seems reasonable to me, as it is only written once at construction time.
> But I think you should at least talk about this change in the commit message, or even do it in a separate patch.

https://searchfox.org/mozilla-central/diff/8a3ec7649b8dcdfd12b7739e3e7945a12a076949/dom/media/MediaCache.cpp#682
There was a null check for mFileCache. I will bring it back.
(In reply to JW Wang [:jwwang] [:jw_wang] from comment #10)
> There was a null check for mFileCache. I will bring it back.

I'm not sure that check is actually needed, as I don't see a way it could ever be null when used in a MediaCache non-static member function (which guarantees the RefPtr is alive).
But if you're not sure, I guess it would be safer to re-add it, and we can examine the situation in another bug -- or not, it's not a big cost anyway.
Comment on attachment 8940054 [details]
Bug 1428242. P3 - use a non-reentrant monitor.

https://reviewboard.mozilla.org/r/210330/#review216072

I think so. I had the same assertion in bug 1374441 while trying to use a non-reentrant monitor. And then I realized this was exactly the issue of bug 1420324. However, we happen to get away with it for unlock never works as expected in MediaCache::ReadCacheFile() because we have double locks in both Read() and ReadAt().
(In reply to Gerald Squelart [:gerald] from comment #11)
> (In reply to JW Wang [:jwwang] [:jw_wang] from comment #10)
> > There was a null check for mFileCache. I will bring it back.
> 
> I'm not sure that check is actually needed, as I don't see a way it could
> ever be null when used in a MediaCache non-static member function (which
> guarantees the RefPtr is alive).
> But if you're not sure, I guess it would be safer to re-add it, and we can
> examine the situation in another bug -- or not, it's not a big cost anyway.

Right, let's err on the side of caution for now and open a new bug to remove the null check if possible.
Thanks for the reviews!
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5f886553c4b
P1 - assert we always take the MediaCache monitor off the main thread. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/90ffd059bdfa
P2 - MediaCache::ReadCacheFile() doesn't need to drop the cache monitor. r=bechen,gerald
https://hg.mozilla.org/integration/autoland/rev/c1bacce33171
P3 - use a non-reentrant monitor. r=bechen,gerald
Duplicate of this bug: 1420324
You need to log in before you can comment on or make changes to this bug.