Bug 61742

*** Bug 61741 has been marked as a duplicate of this bug. ***

Created attachment 95358 [details] Patch

See also http://code.google.com/p/chromium/issues/detail?id=84424

Comment on attachment 95358 [details] 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.

(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.

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?

What's the bug we're fixing? In any case a protected destructor sounds OK to me + scoped_ptr<WebVideoFrameImpl> inside webmediaplayer_impl.cc

Comment on attachment 95358 [details] Patch Err sorry, missed the discussion. This shouldn't be public.

> 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.

(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.

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.

Comment on attachment 95358 [details] Patch (see my last comment)

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.

Chatted with scherkus, he doesn't really care how it is fixed. He's okay with a virtual base class destructor.

Yup I don't really care. Code will likely change at some point anyway.

The commit-queue encountered the following flaky tests while processing attachment 95358 [details]: http/tests/websocket/tests/error-detect.html bug 54012 (author: abarth@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 95358 [details] Patch Clearing flags on attachment: 95358 Committed r88170: <http://trac.webkit.org/changeset/88170>

All reviewed patches have been landed. Closing bug.

You guys forgot to update the comments in WebMediaPlayer.h! (See comment #4.)

scherkus, vrk: Can you update the comment? I don't know what's correct there.

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.