Bug 107488 - Whitelist should also work for the WebVTT ::cue element without an argument
Summary: Whitelist should also work for the WebVTT ::cue element without an argument
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dima Gorbik
URL:
Keywords: InRadar
Depends on:
Blocks: 43668
  Show dependency treegraph
 
Reported: 2013-01-21 17:04 PST by Dima Gorbik
Modified: 2013-01-22 21:51 PST (History)
11 users (show)

See Also:


Attachments
Proposed fix 0.1 (6.81 KB, patch)
2013-01-21 17:16 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff
Proposed fix 0.2 (6.93 KB, patch)
2013-01-21 18:17 PST, Dima Gorbik
darin: review+
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.3 (7.01 KB, patch)
2013-01-21 18:32 PST, Dima Gorbik
darin: review+
Details | Formatted Diff | Diff
Proposed fix 0.4 (7.13 KB, patch)
2013-01-21 18:53 PST, Dima Gorbik
kling: review-
kling: commit-queue-
Details | Formatted Diff | Diff
Proposed fix 0.5 (7.18 KB, patch)
2013-01-22 16:01 PST, Dima Gorbik
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dima Gorbik 2013-01-21 17:04:00 PST
Right now whitelist only works if ::cue() has an argument, but should work in all cases.
Comment 1 Radar WebKit Bug Importer 2013-01-21 17:06:18 PST
<rdar://problem/13057269>
Comment 2 Dima Gorbik 2013-01-21 17:16:23 PST
Created attachment 183857 [details]
Proposed fix 0.1
Comment 3 Dima Gorbik 2013-01-21 18:17:44 PST
Created attachment 183863 [details]
Proposed fix 0.2
Comment 4 Early Warning System Bot 2013-01-21 18:21:17 PST
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
Comment 5 Darin Adler 2013-01-21 18:22:42 PST
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.
Comment 6 Early Warning System Bot 2013-01-21 18:23:52 PST
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
Comment 7 Dima Gorbik 2013-01-21 18:32:19 PST
Created attachment 183865 [details]
Proposed fix 0.3
Comment 8 Darin Adler 2013-01-21 18:35:20 PST
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.
Comment 9 Dima Gorbik 2013-01-21 18:53:32 PST
Created attachment 183867 [details]
Proposed fix 0.4

Nice to have both firstElementChild and nextElementSibling functions.
Thanks for a review!
Comment 10 Andreas Kling 2013-01-21 20:06:37 PST
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.
Comment 11 Dima Gorbik 2013-01-21 20:48:49 PST
(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...
Comment 12 Andreas Kling 2013-01-21 21:11:21 PST
(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()))
Comment 13 Dima Gorbik 2013-01-22 12:59:29 PST
(In reply to comment #12)

Oh, it's a union. My bad, never used them before. We should definitely add an ASSERT to getters.
Comment 14 Dima Gorbik 2013-01-22 16:01:27 PST
Created attachment 184066 [details]
Proposed fix 0.5
Comment 15 WebKit Review Bot 2013-01-22 21:51:44 PST
Comment on attachment 184066 [details]
Proposed fix 0.5

Clearing flags on attachment: 184066

Committed r140505: <http://trac.webkit.org/changeset/140505>
Comment 16 WebKit Review Bot 2013-01-22 21:51:49 PST
All reviewed patches have been landed.  Closing bug.