WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
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.
Top of Page
Format For Printing
XML
Clone This Bug