Bug 107488 – Whitelist should also work for the WebVTT ::cue element without an argument

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

[Dima Gorbik]

Right now whitelist only works if ::cue() has an argument, but should work in all cases.

[Radar WebKit Bug Importer]

<rdar://problem/13057269>

[Dima Gorbik]

Created attachment 183857 [details]
Proposed fix 0.1

[Dima Gorbik]

Created attachment 183863 [details]
Proposed fix 0.2

[Early Warning System Bot]

Comment on attachment 183863 [details]
Proposed fix 0.2

Attachment 183863 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16038274

[Darin Adler]

Comment on attachment 183863 [details]
Proposed fix 0.2

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

> LayoutTests/media/track/track-css-property-whitelist.html:35
>              cueNode = skipNonElements(textTrackDisplayElement(video, 'all-nodes').firstChild);

This should just use firstElementChild instead.

> Source/WebCore/css/RuleSet.cpp:49
>  #include <wtf/MemoryInstrumentationVector.h>
> +#if ENABLE(VIDEO_TRACK)

Need a space before this #if line.

> Source/WebCore/css/RuleSet.cpp:129
> +>>>>>>> whitelist2

Need to delete this.

[Early Warning System Bot]

Comment on attachment 183863 [details]
Proposed fix 0.2

Attachment 183863 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16042283

[Dima Gorbik]

Created attachment 183865 [details]
Proposed fix 0.3

[Darin Adler]

Comment on attachment 183865 [details]
Proposed fix 0.3

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

> LayoutTests/media/track/track-css-property-whitelist.html:19
>          function skipNonElements(root)

Can probably delete this function if it’s not used any more. Anyone using it can probably use firstElementChild or nextElementSibling instead.

[Dima Gorbik]

Created attachment 183867 [details]
Proposed fix 0.4

Nice to have both firstElementChild and nextElementSibling functions.
Thanks for a review!

[Andreas Kling]

Comment on attachment 183867 [details]
Proposed fix 0.4

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

> Source/WebCore/css/RuleSet.cpp:125
> -        if (component->pseudoType() == CSSSelector::PseudoCue)
> +        if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId())

This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast.
In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite.

[Dima Gorbik]

(In reply to comment #10)
> (From update of attachment 183867 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=183867&action=review
> 
> > Source/WebCore/css/RuleSet.cpp:125
> > -        if (component->pseudoType() == CSSSelector::PseudoCue)
> > +        if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId())
> 
> This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast.
> In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite.

Could you elaborate please. Right now I see that m_data.m_tagQName is used when (m_match == Tag). Even if RareData exists (in the constructor). How is that related to the value which is either m_data.m_rareData->m_value if RareData exists either m_data.m_value if it doesn't...

[Andreas Kling]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 183867 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=183867&action=review
> > 
> > > Source/WebCore/css/RuleSet.cpp:125
> > > -        if (component->pseudoType() == CSSSelector::PseudoCue)
> > > +        if (component->pseudoType() == CSSSelector::PseudoCue || selector->value() == TextTrackCue::cueShadowPseudoId())
> > 
> > This looks wrong. If 'component' has (m_match == CSSSelector::Tag), value() should not be called as it will perform an incorrect cast.
> > In fact, CSSSelector::value() should ASSERT(m_match != Tag), just like CSSSelector::tagQName() asserts the opposite.
> 
> Could you elaborate please. Right now I see that m_data.m_tagQName is used when (m_match == Tag). Even if RareData exists (in the constructor). How is that related to the value which is either m_data.m_rareData->m_value if RareData exists either m_data.m_value if it doesn't...

First of all, it's calling value() on 'selector' in every iteration, not on 'component' as it should.

With that fixed, the problem is that m_data's m_value/m_rareData/m_tagQName all share the same memory location. (m_match == Tag) means that it's QualifiedNameImpl* is stored there, otherwise it's either a RareData* or an AtomicStringImpl*.

Calling value() will treat whatever is in the m_data union as if it's either AtomicStringImpl* or RareData*, even though it could also be a QualifiedNameImpl*.

Basically, the check should look like this:

if (component->pseudoType() == CSSSelector::PseudoCue || (component->m_match != Tag && component->value() == TextTrackCue::cueShadowPseudoId()))

[Dima Gorbik]

(In reply to comment #12)

Oh, it's a union. My bad, never used them before. We should definitely add an ASSERT to getters.

[Dima Gorbik]

Created attachment 184066 [details]
Proposed fix 0.5

[WebKit Review Bot]

Comment on attachment 184066 [details]
Proposed fix 0.5

Clearing flags on attachment: 184066

Committed r140505: <http://trac.webkit.org/changeset/140505>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.