Bug 105642 – Dubious cast from TextTrackCueBox to HTMLDivElement.

• Status: NEW
• Component: WebCore Misc.

[Thomas Sepez]

In TextTrackCueBox::TextTrackCueBox(), there is an initialization of the form : HTMLElement(divTag, document).
Under V8, Initializing an element using divTag gives V8HTMLElementWrapperFactory the right to invoke createHTMLDivElementWrapper() upon the element.
createHTMLDivElementWrapper() invokes static_cast<HTMLDivElement*>(element).

Note however, the TextTrackCueBox inherits from HTMLElement, not HTMLDivElement, so the cast is wrong.  Presently, this does no harm at the machine code level, since HTMLDivElement doesn't modify the layout of the underlying memory.  But something could change, and then you won't be ok.

The fix would be to change to the proper base class in TextTrackCueBox.cpp.   Looking at https://bugs.webkit.org/show_bug.cgi?id=79751, this was initially the design but went off the rails at comment 34,
when rather than resolving the underlying issue, this bug was introduced as part of a workaround.

[Victor Carbune]

Thanks for pointing this out.

(In reply to comment #0)
> The fix would be to change to the proper base class in TextTrackCueBox.cpp.   Looking at https://bugs.webkit.org/show_bug.cgi?id=79751, this was initially the design but went off the rails at comment 34,
> when rather than resolving the underlying issue, this bug was introduced as part of a workaround.
I remember what was the issue that determined me to this.

Most of the .mm config files in WebView included HTMLElement, but not
HTMLDivElement and therefore I wasn't completely sure it's appropriate to
simply include HTMLDivElement because of WebVTT.

So would it be appropriate to actually do just do this directly?

[Victor Carbune]

Created attachment 184771 [details]
Trivial fix

Checking what the build-bots have to say

[Build Bot]

Comment on attachment 184771 [details]
Trivial fix

Attachment 184771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16118457

[Ryosuke Niwa]

Comment on attachment 184771 [details]
Trivial fix

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

> Source/WebCore/ChangeLog:8
> +        Trivial change.

Please explain why we're making this change. r- due to the lack of explanation.

> Source/WebCore/html/track/TextTrackCue.cpp:99
> -    : HTMLElement(divTag, document)
> +    : HTMLDivElement(divTag, document)

Why are we making this change? Does the spec say we should do this? If not, why should we do this?

[Victor Carbune]

(In reply to comment #4)
> (From update of attachment 184771 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184771&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Trivial change.
> 
> Please explain why we're making this change. r- due to the lack of explanation.
As I mentioned in the comment, I wanted to see whether the build crashes on Mac
(I'm sorry for not marking r- after seeing the output)

> > Source/WebCore/html/track/TextTrackCue.cpp:99
> > -    : HTMLElement(divTag, document)
> > +    : HTMLDivElement(divTag, document)
> 
> Why are we making this change? Does the spec say we should do this? If not, why should we do this?

This is not a behavior change - as pointed in the first comment of this bug,
it's wrong to use the divTag and inherit from HTMLElement instead of 
HTMLDivElement.

Cues are reusing for rendering as much as possible existing HTML elements
(such as this div), except for the last step when multiple such boxes (divs) 
overlap.

[Ryosuke Niwa]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 184771 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=184771&action=review
> > 
> > > Source/WebCore/ChangeLog:8
> > > +        Trivial change.
> > 
> > Please explain why we're making this change. r- due to the lack of explanation.
> As I mentioned in the comment, I wanted to see whether the build crashes on Mac
> (I'm sorry for not marking r- after seeing the output)

Please don't set r- if you're not a reviewer. Instead, simply remove the flag if you don't want reviewers to review the patch.

> > > Source/WebCore/html/track/TextTrackCue.cpp:99
> > > -    : HTMLElement(divTag, document)
> > > +    : HTMLDivElement(divTag, document)
> > 
> > Why are we making this change? Does the spec say we should do this? If not, why should we do this?
> 
> This is not a behavior change - as pointed in the first comment of this bug,
> it's wrong to use the divTag and inherit from HTMLElement instead of 
> HTMLDivElement.

That's certainly not true, right? We were creating a wrong wrapper in V8. That's certainly visible to JavaScript is clearly an observable bug as I understand it. e.g. prototype chain. We need a test.

> Cues are reusing for rendering as much as possible existing HTML elements
> (such as this div), except for the last step when multiple such boxes (divs) 
> overlap.

I'm not convinced that the right fix is to change the parent class to HTMLDivElement. I suspect the problem is that we're using divTag. We should probably be creating some other QualifiedName and use that instead.

[Victor Carbune]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > (From update of attachment 184771 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=184771&action=review
> > > 
> > > > Source/WebCore/ChangeLog:8
> > > > +        Trivial change.
> > > 
> > > Please explain why we're making this change. r- due to the lack of explanation.
> > As I mentioned in the comment, I wanted to see whether the build crashes on Mac
> > (I'm sorry for not marking r- after seeing the output)
> 
> Please don't set r- if you're not a reviewer. Instead, simply remove the flag if you don't want reviewers to review the patch.
> 
> > > > Source/WebCore/html/track/TextTrackCue.cpp:99
> > > > -    : HTMLElement(divTag, document)
> > > > +    : HTMLDivElement(divTag, document)
> > > 
> > > Why are we making this change? Does the spec say we should do this? If not, why should we do this?
> > 
> > This is not a behavior change - as pointed in the first comment of this bug,
> > it's wrong to use the divTag and inherit from HTMLElement instead of 
> > HTMLDivElement.
> 
> That's certainly not true, right? We were creating a wrong wrapper in V8. That's certainly visible to JavaScript is clearly an observable bug as I understand it. e.g. prototype chain. We need a test.

I meant purely from the final outcome regarding cue rendering. It doesn't 
change how they are displayed.

The TextTrackCueBox is used only internally and your observation is valid, indeed.
 
> > Cues are reusing for rendering as much as possible existing HTML elements
> > (such as this div), except for the last step when multiple such boxes (divs) 
> > overlap.
> 
> I'm not convinced that the right fix is to change the parent class to HTMLDivElement. I suspect the problem is that we're using divTag. We should probably be creating some other QualifiedName and use that instead.

Makes sense, if even more custom logic appears on top of elements that have the
divTag (and would rely on a specific memory layout).