105642 2012-12-21 10:44:49 -0800 Dubious cast from TextTrackCueBox to HTMLDivElement. 2013-01-25 12:46:56 -0800 1 1 1 Unclassified WebKit WebCore Misc. 528+ (Nightly build) All Unspecified NEW P2 Trivial --- 106608 1 tsepez vcarbune buildbot eric.carlson japhet mjs ojan.autocc rniwa tony vcarbune webkit.review.bot oldest_to_newest 796373 0 tsepez 2012-12-21 10:44:49 -0800 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. 796393 1 vcarbune 2012-12-21 11:19:18 -0800 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? 816695 2 184771 vcarbune 2013-01-25 10:01:44 -0800 Created attachment 184771 Trivial fix Checking what the build-bots have to say 816737 3 184771 buildbot 2013-01-25 10:45:24 -0800 Comment on attachment 184771 Trivial fix Attachment 184771 did not pass mac-wk2-ews (mac-wk2): Output: http://queues.webkit.org/results/16118457 816742 4 184771 rniwa 2013-01-25 10:47:43 -0800 Comment on attachment 184771 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? 816769 5 vcarbune 2013-01-25 11:07:57 -0800 (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. 816821 6 rniwa 2013-01-25 12:04:35 -0800 (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. 816850 7 vcarbune 2013-01-25 12:46:56 -0800 (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). 184771 2013-01-25 10:01:44 -0800 2013-01-25 10:47:43 -0800 Trivial fix bug-105642-20130125195834.patch text/plain 2121 vcarbune U3VidmVyc2lvbiBSZXZpc2lvbjogMTQwNzE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZmNhNzBkMTM2NzFlNWQ4 NjI5MTk1YTliNjUzZDBjZDE5MTI3ZWIxZS4uNzIwY2M4YjAwM2Y4MTlkM2FlMzZmMzcxMTlhNjVj OWU2ODViNWQ0ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDEzLTAxLTI1ICBWaWN0 b3IgQ2FyYnVuZSAgPHZjYXJidW5lQGNocm9taXVtLm9yZz4KKworICAgICAgICBEdWJpb3VzIGNh c3QgZnJvbSBUZXh0VHJhY2tDdWVCb3ggdG8gSFRNTERpdkVsZW1lbnQuCisgICAgICAgIGh0dHBz Oi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDU2NDIKKworICAgICAgICBSZXZp ZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBUcml2aWFsIGNoYW5nZS4KKworICAg ICAgICAqIGh0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OlRl eHRUcmFja0N1ZUJveDo6VGV4dFRyYWNrQ3VlQm94KToKKyAgICAgICAgKiBodG1sL3RyYWNrL1Rl eHRUcmFja0N1ZS5oOgorCiAyMDEzLTAxLTI0ICBKZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUu Y29tPgogCiAgICAgICAgIFVucmV2aWV3ZWQgYnVpbGQgZml4IGZvciBNYWMvTGlvbi4KZGlmZiAt LWdpdCBhL1NvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcCBiL1NvdXJj ZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmNwcAppbmRleCAzODQ1YTc0OWU5NTQ1 OGI0ZTc5NTQ1OTcxMTRlY2MyZjk3MGVhN2QxLi5lZDdmYTE1ZDdlN2RjM2RmNGNiYjBmNmMzOGNj MzMyNzNhYTBlZTdmIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1RleHRU cmFja0N1ZS5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvaHRtbC90cmFjay9UZXh0VHJhY2tDdWUu Y3BwCkBAIC05Niw3ICs5Niw3IEBAIHN0YXRpYyBjb25zdCBTdHJpbmcmIHZlcnRpY2FsR3Jvd2lu Z1JpZ2h0S2V5d29yZCgpCiAvLyAtLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tCiAKIFRleHRU cmFja0N1ZUJveDo6VGV4dFRyYWNrQ3VlQm94KERvY3VtZW50KiBkb2N1bWVudCwgVGV4dFRyYWNr Q3VlKiBjdWUpCi0gICAgOiBIVE1MRWxlbWVudChkaXZUYWcsIGRvY3VtZW50KQorICAgIDogSFRN TERpdkVsZW1lbnQoZGl2VGFnLCBkb2N1bWVudCkKICAgICAsIG1fY3VlKGN1ZSkKIHsKICAgICBz ZXRQc2V1ZG8odGV4dFRyYWNrQ3VlQm94U2hhZG93UHNldWRvSWQoKSk7CmRpZmYgLS1naXQgYS9T b3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1RleHRUcmFja0N1ZS5oIGIvU291cmNlL1dlYkNvcmUv aHRtbC90cmFjay9UZXh0VHJhY2tDdWUuaAppbmRleCBjYzE2ZjA3OWZiZDE5NTk3NTAxYzdiNGM5 ZmVmZjU1Y2IzOTRkZmQ0Li4yOWFkNTkwYTVjYzA4YWZlMDBmZGRlNWE5ZTg4NzY3M2I3NjllMjA5 IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL3RyYWNrL1RleHRUcmFja0N1ZS5oCisr KyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvdHJhY2svVGV4dFRyYWNrQ3VlLmgKQEAgLTM1LDcgKzM1 LDcgQEAKICNpZiBFTkFCTEUoVklERU9fVFJBQ0spCiAKICNpbmNsdWRlICJFdmVudFRhcmdldC5o IgotI2luY2x1ZGUgIkhUTUxFbGVtZW50LmgiCisjaW5jbHVkZSAiSFRNTERpdkVsZW1lbnQuaCIK ICNpbmNsdWRlICJUZXh0VHJhY2suaCIKICNpbmNsdWRlIDx3dGYvUGFzc093blB0ci5oPgogI2lu Y2x1ZGUgPHd0Zi9SZWZDb3VudGVkLmg+CkBAIC01MCw3ICs1MCw3IEBAIGNsYXNzIFRleHRUcmFj a0N1ZTsKIAogLy8gLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLS0tLQogCi1jbGFzcyBUZXh0VHJh Y2tDdWVCb3ggOiBwdWJsaWMgSFRNTEVsZW1lbnQgeworY2xhc3MgVGV4dFRyYWNrQ3VlQm94IDog cHVibGljIEhUTUxEaXZFbGVtZW50IHsKIHB1YmxpYzoKICAgICBzdGF0aWMgUGFzc1JlZlB0cjxU ZXh0VHJhY2tDdWVCb3g+IGNyZWF0ZShEb2N1bWVudCogZG9jdW1lbnQsIFRleHRUcmFja0N1ZSog Y3VlKQogICAgIHsK