NEW 105642
Dubious cast from TextTrackCueBox to HTMLDivElement.
https://bugs.webkit.org/show_bug.cgi?id=105642
Summary Dubious cast from TextTrackCueBox to HTMLDivElement.
Thomas Sepez
Reported 2012-12-21 10:44:49 PST
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.
Attachments
Trivial fix (2.07 KB, patch)
2013-01-25 10:01 PST, Victor Carbune
rniwa: review-
buildbot: commit-queue-
Victor Carbune
Comment 1 2012-12-21 11:19:18 PST
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
Comment 2 2013-01-25 10:01:44 PST
Created attachment 184771 [details] Trivial fix Checking what the build-bots have to say
Build Bot
Comment 3 2013-01-25 10:45:24 PST
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 4 2013-01-25 10:47:43 PST
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
Comment 5 2013-01-25 11:07:57 PST
(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
Comment 6 2013-01-25 12:04:35 PST
(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
Comment 7 2013-01-25 12:46:56 PST
(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).
Note You need to log in before you can comment on or make changes to this bug.