61742 2011-05-30 11:32:31 -0700 [chromium] Give WebVideoFrame a virtual destructor 2011-06-13 14:47:47 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 thakis thakis akalin commit-queue fishd hclam scherkus vrk oldest_to_newest 412306 0 thakis 2011-05-30 11:32:31 -0700 [chromium] Give WebVideoFrame a virtual destructor 412308 1 thakis 2011-05-30 11:33:01 -0700 *** Bug 61741 has been marked as a duplicate of this bug. *** 412309 2 95358 thakis 2011-05-30 11:34:51 -0700 Created attachment 95358 Patch 412310 3 thakis 2011-05-30 11:35:14 -0700 See also http://code.google.com/p/chromium/issues/detail?id=84424 412978 4 95358 fishd 2011-05-31 16:33:00 -0700 Comment on attachment 95358 Patch The comments in WebMediaPlayer.h say: // This function returns a pointer to a WebVideoFrame, which is // a WebKit wrapper for a video frame in chromium. This places a lock // on the frame in chromium, and calls to this method should always be // followed with a call to putCurrentFrame(). The ownership of this object // is not transferred to the caller, and the caller should not free the // returned object. virtual WebVideoFrame* getCurrentFrame() { return 0; } That tells me that WebVideoFrame should actually have a protected destructor. It seems like the Chromium code is violating the API contract in other words. Or, the comment is wrong. 412992 5 vrk 2011-05-31 16:51:03 -0700 (In reply to comment #4) > (From update of attachment 95358 [details]) > The comments in WebMediaPlayer.h say: > > // This function returns a pointer to a WebVideoFrame, which is > // a WebKit wrapper for a video frame in chromium. This places a lock > // on the frame in chromium, and calls to this method should always be > // followed with a call to putCurrentFrame(). The ownership of this object > // is not transferred to the caller, and the caller should not free the > // returned object. > virtual WebVideoFrame* getCurrentFrame() { return 0; } > > That tells me that WebVideoFrame should actually have a protected destructor. > It seems like the Chromium code is violating the API contract in other words. > > Or, the comment is wrong. 412995 6 vrk 2011-05-31 16:56:34 -0700 Whoops didn't mean to submit last comment! Sorry for spam. As far as I can tell, darin is right and the comment in WebVideoFrame is right, so yes, the WebVideoFrame destructor should be protected. I think we could change webkit/glue/webmediaplayer_impl.cc to have a scoped_refptr<WebVideoFrame> current_frame_ field that gets set/reset every call to getCurrentFrame() and putCurrentFrame(). @scherkus/hclam thoughts? 413001 7 scherkus 2011-05-31 17:03:15 -0700 What's the bug we're fixing? In any case a protected destructor sounds OK to me + scoped_ptr<WebVideoFrameImpl> inside webmediaplayer_impl.cc 413089 8 95358 jamesr 2011-05-31 19:36:35 -0700 Comment on attachment 95358 Patch Err sorry, missed the discussion. This shouldn't be public. 414176 9 thakis 2011-06-02 09:09:27 -0700 > That tells me that WebVideoFrame should actually have a protected destructor. fishd, can you comment on http://codereview.chromium.org/7094005/ ? The protected non-virtual destructor pattern is technically only correct if implementing classes are marked as final, but folks on that review feel that that's too complicated. 414970 10 fishd 2011-06-03 10:01:45 -0700 (In reply to comment #9) > > That tells me that WebVideoFrame should actually have a protected destructor. > > fishd, can you comment on http://codereview.chromium.org/7094005/ ? The protected non-virtual destructor pattern is technically only correct if implementing classes are marked as final, but folks on that review feel that that's too complicated. Note: The issue of whether or not this destructor should be virtual or non-virtual is not the main issue in _this_ bug. The main issue here is that no one should be deleting a WebVideoFrame pointer. We can enforce that by giving WebVideoFrame a protected destructor. 415134 11 thakis 2011-06-03 13:33:57 -0700 fishd, vrk, scherkus: Making the destructor protected is not trivial. Here's the method where the delete happens ( http://google.com/codesearch/p?hl=en#hfE6470xZHk/webkit/glue/webmediaplayer_impl.cc&q=webmediaplayerimpl&d=2&l=599 ): void WebMediaPlayerImpl::putCurrentFrame( WebKit::WebVideoFrame* web_video_frame) { if (web_video_frame) { scoped_refptr<media::VideoFrame> video_frame = WebVideoFrameImpl::toVideoFrame(web_video_frame); proxy_->PutCurrentFrame(video_frame); delete web_video_frame; // <- XXX HERE XXX } } Since this overrides a method of WebMediaPlayer with the following interface ( http://codesearch.google.com/codesearch/p?hl=en#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKit/chromium/public/WebMediaPlayer.h&q=webmediaplayer.h&exact_package=chromium&sa=N&cd=1&ct=rc ) virtual void putCurrentFrame(WebVideoFrame*) { } the argument type must be WebVideoFrame, not WebVideoFrameImpl. WebMediaPlayerImpl could do a static_cast to WebVideoFrameImpl, but that seems worse than to just make this destructor virtual. Let me know what you want me to do here. 415193 12 95358 thakis 2011-06-03 14:12:03 -0700 Comment on attachment 95358 Patch (see my last comment) 415344 13 thakis 2011-06-03 18:09:46 -0700 vrk, scherkus: Ping. This CL as is fixes a real bug, and unblocks me from rolling clang. If you want me to do something else, please tell me what. 415346 14 akalin 2011-06-03 18:14:12 -0700 Chatted with scherkus, he doesn't really care how it is fixed. He's okay with a virtual base class destructor. 415349 15 scherkus 2011-06-03 18:30:17 -0700 Yup I don't really care. Code will likely change at some point anyway. 415867 16 commit-queue 2011-06-06 09:46:54 -0700 The commit-queue encountered the following flaky tests while processing attachment 95358: http/tests/websocket/tests/error-detect.html bug 54012 (author: abarth@webkit.org) The commit-queue is continuing to process your patch. 415868 17 95358 commit-queue 2011-06-06 09:48:23 -0700 Comment on attachment 95358 Patch Clearing flags on attachment: 95358 Committed r88170: <http://trac.webkit.org/changeset/88170> 415869 18 commit-queue 2011-06-06 09:48:29 -0700 All reviewed patches have been landed. Closing bug. 417679 19 fishd 2011-06-08 16:37:22 -0700 You guys forgot to update the comments in WebMediaPlayer.h! (See comment #4.) 417684 20 thakis 2011-06-08 16:41:23 -0700 scherkus, vrk: Can you update the comment? I don't know what's correct there. 419892 21 scherkus 2011-06-13 14:47:47 -0700 I've lost some context on this (went on vacation, flushed out cache, etc..) but is there any reason why we can't have a virtual-yet-protected dtor? the comment is correct but the code in chromium is a bit kludgy: void WebMediaPlayerImpl::putCurrentFrame( WebKit::WebVideoFrame* web_video_frame) { if (web_video_frame) { scoped_refptr<media::VideoFrame> video_frame( WebVideoFrameImpl::toVideoFrame(web_video_frame)); proxy_->PutCurrentFrame(video_frame); delete web_video_frame; } } ...I believe a combo of that change + a static_cast<WebVideoFrameImpl> inside chromium land (as jamesr/fishd mentioned) be the correct course of action here so that we delete the Impl version of the pointer. 95358 2011-05-30 11:34:51 -0700 2011-06-06 09:48:23 -0700 Patch bug-61742-20110530203448.patch text/plain 1389 thakis U3VidmVyc2lvbiBSZXZpc2lvbjogODc2MjcKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hy b21pdW0vQ2hhbmdlTG9nIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKaW5kZXgg OGMwZjIwMGNmZTI3NzdmMzVjNTMwNTE1ZWY5ZGVkMGMxMzBjY2E3ZC4uYTg4MTBmNTYwZjNlNjAy ODI4ZGIzNTE1ODgzNTdjODJiZGZmYzA2YiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdC9jaHJv bWl1bS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9DaGFuZ2VMb2cKQEAg LTEsMyArMSwxNyBAQAorMjAxMS0wNS0zMCAgTmljbyBXZWJlciAgPHRoYWtpc0BjaHJvbWl1bS5v cmc+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgd2Vi a2l0L2dsdWUvd2VibWVkaWFwbGF5ZXJfaW1wbC5jYyBkZWxldGVzIGFuIG9iamVjdCBvZiB0eXBl CisgICAgICAgIFdlYlZpZGVvRnJhbWUsIHdoaWNoIG1lYW5zIHRoaXMgcGF0Y2ggZml4ZXMgYSBy ZWFsIGJ1Zywgbm90IGp1c3QKKyAgICAgICAgYSB0aGVvcmV0aWNhbCBvbmUuCisKKyAgICAgICAg W2Nocm9taXVtXSBHaXZlIFdlYlZpZGVvRnJhbWUgYSB2aXJ0dWFsIGRlc3RydWN0b3IKKyAgICAg ICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTYxNzQyCisKKyAgICAg ICAgKiBwdWJsaWMvV2ViVmlkZW9GcmFtZS5oOgorICAgICAgICAoV2ViS2l0OjpXZWJWaWRlb0Zy YW1lOjp+V2ViVmlkZW9GcmFtZSk6CisKIDIwMTEtMDUtMjcgIEpvY2hlbiBFaXNpbmdlciAgPGpv Y2hlbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgQWRhbSBCYXJ0aC4KZGlm ZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYlZpZGVvRnJhbWUuaCBi L1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vcHVibGljL1dlYlZpZGVvRnJhbWUuaAppbmRleCA1OGYx MTExMWFiODg4ZDE2NjQ3NTBjZjAxYzRmMWEzM2U2M2JmMWU5Li5jYzg4Mjk5ZjM2YmM4MjRlZDI2 OWU4NDZjMDk3ODNlOWQxNzczNmQ4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0L2Nocm9taXVt L3B1YmxpYy9XZWJWaWRlb0ZyYW1lLmgKKysrIGIvU291cmNlL1dlYktpdC9jaHJvbWl1bS9wdWJs aWMvV2ViVmlkZW9GcmFtZS5oCkBAIC01Niw2ICs1Niw3IEBAIHB1YmxpYzoKICAgICAgICAgU3Vy ZmFjZVR5cGVUZXh0dXJlLAogICAgIH07CiAKKyAgICB2aXJ0dWFsIH5XZWJWaWRlb0ZyYW1lKCkg eyB9CiAgICAgdmlydHVhbCBTdXJmYWNlVHlwZSBzdXJmYWNlVHlwZSgpIGNvbnN0ID0gMDsKICAg ICB2aXJ0dWFsIEZvcm1hdCBmb3JtYXQoKSBjb25zdCA9IDA7CiAgICAgdmlydHVhbCB1bnNpZ25l ZCB3aWR0aCgpIGNvbnN0ID0gMDsK