Bug 208904 – Safari sometimes crashes when switch video into PiP mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Peng Liu]

Safari sometimes crash when switch video into PiP mode

[Peng Liu]

<rdar://problem/59837202>

[Peng Liu]

Created attachment 393203 [details]
WIP patch

[Peng Liu]

The assertion "ASSERT(clipRectsContext.rootLayer == m_clipRectsCache->m_clipRectsRoot[clipRectsType]);" in RenderLayer.cpp fails with the following stack trace:

ASSERTION FAILED: clipRectsContext.rootLayer == m_clipRectsCache->m_clipRectsRoot[clipRectsType]
./rendering/RenderLayer.cpp(5702) : Ref<WebCore::ClipRects> WebCore::RenderLayer::updateClipRects(const WebCore::RenderLayer::ClipRectsContext &)
1   0x2c7e18219 WTFCrash
2   0x2acf32f2b WTFCrashWithInfo(int, char const*, char const*, int)
3   0x2b0b8a1f5 WebCore::RenderLayer::updateClipRects(WebCore::RenderLayer::ClipRectsContext const&)
4   0x2b0b8a7d2 WebCore::RenderLayer::parentClipRects(WebCore::RenderLayer::ClipRectsContext const&) const
5   0x2b0b7f99e WebCore::RenderLayer::backgroundClipRect(WebCore::RenderLayer::ClipRectsContext const&) const
6   0x2b0b85cd9 WebCore::RenderLayer::calculateRects(WebCore::RenderLayer::ClipRectsContext const&, WebCore::LayoutRect const&, WebCore::LayoutRect&, WebCore::ClipRect&, WebCore::ClipRect&, WebCore::LayoutSize const&) const
7   0x2b0b83834 WebCore::RenderLayer::collectFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&, WebCore::RenderLayer const*, WebCore::LayoutRect const&, WebCore::RenderLayer::PaginationInclusionMode, WebCore::ClipRectsType, WebCore::OverlayScrollbarSizeRelevancy, WebCore::ShouldRespectOverflowClip, WebCore::LayoutSize const&, WebCore::LayoutRect const*, WebCore::ShouldApplyRootOffsetToFragments)
8   0x2b0b80dfa WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
9   0x2b0b80184 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
10  0x2b0b7f13b WebCore::RenderLayer::paintLayerWithEffects(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
11  0x2b0b7e37b WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
12  0x2b0b845e4 WebCore::RenderLayer::paintList(WebCore::RenderLayer::LayerList, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
13  0x2b0b810f4 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
14  0x2b0b80184 WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
15  0x2b0b7f13b WebCore::RenderLayer::paintLayerWithEffects(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
16  0x2b0b7e37b WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
17  0x2b0b7e085 WebCore::RenderLayer::paint(WebCore::GraphicsContext&, WebCore::LayoutRect const&, WebCore::LayoutSize const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::RenderObject*, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>, WebCore::RenderLayer::SecurityOriginPaintPolicy)
18  0x2afd68df2 WebCore::MediaControlTextTrackContainerElement::createTextTrackRepresentationImage()
19  0x2afd68fa6 non-virtual thunk to WebCore::MediaControlTextTrackContainerElement::createTextTrackRepresentationImage()
20  0x2ae6b36af WebCore::TextTrackRepresentationCocoa::update()
21  0x2afd679ec WebCore::MediaControlTextTrackContainerElement::updateTextTrackRepresentation()
22  0x2afd68fe9 WebCore::MediaControlTextTrackContainerElement::textTrackRepresentationBoundsChanged(WebCore::IntRect const&)
23  0x2ae6bf5a5 WebCore::TextTrackRepresentationCocoa::boundsChanged()::$_0::operator()() const
24  0x2ae6bf52e WTF::Detail::CallableWrapper<WebCore::TextTrackRepresentationCocoa::boundsChanged()::$_0, void>::call()
25  0x2acf456d2 WTF::Function<void ()>::operator()() const
26  0x2ad1f0c05 WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'()::operator()() const
27  0x2ad1f0a3e WTF::Detail::CallableWrapper<WebCore::GenericTaskQueue<WebCore::Timer>::enqueueTask(WTF::Function<void ()>&&)::'lambda'(), void>::call()
28  0x2acf456d2 WTF::Function<void ()>::operator()() const
29  0x2b04e36ef WebCore::TaskDispatcher<WebCore::Timer>::dispatchOneTask()
30  0x2b04e33c5 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimerFired()
31  0x2b04e7c81 WebCore::TaskDispatcher<WebCore::Timer>::sharedTimer()::$_1::operator()() const

[zalan]

This looks to me the wrong direction. Is there a reason why we don't address this issue properly?

[zalan]

(In reply to Peng Liu from comment #2)
> Created attachment 393203 [details]
> WIP patch
If it's a WIP patch, why the r?

[Peng Liu]

(In reply to zalan from comment #5)
> (In reply to Peng Liu from comment #2)
> > Created attachment 393203 [details]
> > WIP patch
> If it's a WIP patch, why the r?

I am not sure whether the assertion is valid or not in the specific scenario and would like to get some comments. The patch works by removing it.

[Simon Fraser (smfr)]

Comment on attachment 393203 [details]
WIP patch

The assertion still indicates that this isn't right.

[Peng Liu]

Created attachment 393324 [details]
WIP patch v2

[Peng Liu]

Created attachment 393338 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 393338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393338&action=review

> Source/WebCore/dom/Document.h:1807
> +    WeakPtr<HTMLMediaElement> m_mediaElementShowingTextTrack;

Is there only ever one?

> Source/WebCore/html/HTMLMediaElement.cpp:6385
> +    if (!m_haveVisibleTextTrack && !hasMediaControls())
> +        return;
> +    if (!hasMediaControls() && !createMediaControls())
> +        return;

The two adjacent !hasMediaControls() checks are weird.

> Source/WebCore/html/shadow/MediaControlElements.h:489
> +    void layoutIfNeeded();
> +    void paintIfNeeded();

These are not things that elements should know about. Renderers layout and paint. Elements are just input into render tree creation (along with style).

> Source/WebCore/html/shadow/MediaControls.cpp:426
> +void MediaControls::paintTextTrackIfNeeded()
> +{
> +    if (m_textDisplayContainer)
> +        m_textDisplayContainer->paintIfNeeded();
> +}

Let's call this "update text track image" or something.

> Source/WebCore/rendering/RenderMediaControlElements.cpp:106
>      LayoutStateDisabler layoutStateDisabler(view().frameView().layoutContext());

I'm really thrown by the fact that this class is called RenderTextTrackContainerElement. It's not an Element. Can we rename it in a first step, to RenderTextTrackContainer ?

> Source/WebCore/rendering/RenderMediaControlElements.cpp:107
> +    static_cast<MediaControlTextTrackContainerElement*>(element())->layoutIfNeeded();

Elements don't do layout.

[zalan]

Comment on attachment 393338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393338&action=review

>> Source/WebCore/rendering/RenderMediaControlElements.cpp:107
>> +    static_cast<MediaControlTextTrackContainerElement*>(element())->layoutIfNeeded();
> 
> Elements don't do layout.

I am sure we can do better than static_cast<>->layout().

[Peng Liu]

Created attachment 393399 [details]
Patch

[Peng Liu]

Comment on attachment 393338 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393338&action=review

>> Source/WebCore/dom/Document.h:1807
>> +    WeakPtr<HTMLMediaElement> m_mediaElementShowingTextTrack;
> 
> Is there only ever one?

Yes, there is only one video element showing text track (in video fullscreen or PiP mode).

>> Source/WebCore/html/HTMLMediaElement.cpp:6385
>> +        return;
> 
> The two adjacent !hasMediaControls() checks are weird.

Yes. I merged them.

>> Source/WebCore/html/shadow/MediaControlElements.h:489
>> +    void paintIfNeeded();
> 
> These are not things that elements should know about. Renderers layout and paint. Elements are just input into render tree creation (along with style).

Right. Renamed them to updateTextTrackSizeAndStyle() and updateTextTrackRepresentationImageIfNeeded().

>> Source/WebCore/html/shadow/MediaControls.cpp:426
>> +}
> 
> Let's call this "update text track image" or something.

OK. Renamed it to updateTextTrackRepresentationImageIfNeeded().

>> Source/WebCore/rendering/RenderMediaControlElements.cpp:106
>>      LayoutStateDisabler layoutStateDisabler(view().frameView().layoutContext());
> 
> I'm really thrown by the fact that this class is called RenderTextTrackContainerElement. It's not an Element. Can we rename it in a first step, to RenderTextTrackContainer ?

Good idea. Let's rename it to RenderMediaControlTextTrackContainer.
Filed a bug webkit.org/b/209008 to cleanup this file.

>>> Source/WebCore/rendering/RenderMediaControlElements.cpp:107
>>> +    static_cast<MediaControlTextTrackContainerElement*>(element())->layoutIfNeeded();
>> 
>> Elements don't do layout.
> 
> I am sure we can do better than static_cast<>->layout().

Ditto. Renamed the functions.

[Simon Fraser (smfr)]

Comment on attachment 393399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393399&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:5814
> +        document().clearMediaElementShowingTextTrack();

I don't see a call to clearMediaElementShowingTextTrack() if the media element is removed from the document. There probably needs to be another call from HTMLMediaElement::removedFromAncestor() or something.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1216
> +void MediaControlTextTrackContainerElement::updateTextTrackSizeAndStyle()

This is called "size and style" but it doesn't do anything with style (which is a good thing).

> Source/WebCore/html/shadow/MediaControlElements.cpp:1237
> +void MediaControlTextTrackContainerElement::updateTextTrackRepresentationImageIfNeeded()
> +{
> +    if (!m_needsPaint)
> +        return;
> +
> +    m_needsPaint = false;
> +
> +    // We can only call m_textTrackRepresentation->update() after the layout
> +    // is clean, because m_textTrackRepresentation->update() will call
> +    // createTextTrackRepresentationImage() to paint the subtree of
> +    // RenderTextTrackContainerElement to an image buffer.
> +    if (m_textTrackRepresentation)
> +        m_textTrackRepresentation->update();

m_needsPaint should be named to reflect the functions that get called when it changes state. That's called "update" here, which is vague. It's more like "generate text track representation". The fact that it's painting into an image buffer is implementation detail.

> Source/WebCore/html/shadow/MediaControlElements.cpp:1447
>      updateActiveCuesFontSize();
> -    updateDisplay();
>      updateTextStrokeStyle();

It looks like there's style changing happening here, which should really happen along with other style changes on the document (before styleResolve). If so, please file a bug to fix.

[Peng Liu]

Created attachment 393507 [details]
Patch for landing

[Peng Liu]

Comment on attachment 393399 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=393399&action=review

>> Source/WebCore/html/HTMLMediaElement.cpp:5814
>> +        document().clearMediaElementShowingTextTrack();
> 
> I don't see a call to clearMediaElementShowingTextTrack() if the media element is removed from the document. There probably needs to be another call from HTMLMediaElement::removedFromAncestor() or something.

The text track representation is used to show captions in video fullscreen or picture-in-picture mode, and only one media element can be in fullscreen or picture-in-picture mode (for now). When a media element exit fullscreen (the element will exit fullscreen or picture-in-picture if it is removed from the document) it will be removed from the document.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1216
>> +void MediaControlTextTrackContainerElement::updateTextTrackSizeAndStyle()
> 
> This is called "size and style" but it doesn't do anything with style (which is a good thing).

I found we do not need to call this function from RenderMediaControlTextTrackContainer::layout(), so I removed this function.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1237
>> +        m_textTrackRepresentation->update();
> 
> m_needsPaint should be named to reflect the functions that get called when it changes state. That's called "update" here, which is vague. It's more like "generate text track representation". The fact that it's painting into an image buffer is implementation detail.

Good points. I renamed m_needsPaint to m_needsGenerateTextTrackRepresentation and updated the comments.

>> Source/WebCore/html/shadow/MediaControlElements.cpp:1447
>>      updateTextStrokeStyle();
> 
> It looks like there's style changing happening here, which should really happen along with other style changes on the document (before styleResolve). If so, please file a bug to fix.

Right. I refactored the code to avoid updating styles during layout.

[WebKit Commit Bot]

Comment on attachment 393507 [details]
Patch for landing

Clearing flags on attachment: 393507

Committed r258434: <https://trac.webkit.org/changeset/258434>

[Jer Noble]

*** Bug 208864 has been marked as a duplicate of this bug. ***