Bug 61742 – [chromium] Give WebVideoFrame a virtual destructor

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Nico Weber]

[chromium] Give WebVideoFrame a virtual destructor

[Nico Weber]

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

[Nico Weber]

Created attachment 95358 [details]
Patch

[Nico Weber]

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

[Darin Fisher (:fishd, Google)]

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.

[Victoria Kirst]

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

[Victoria Kirst]

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?

[Andrew Scherkus]

What's the bug we're fixing?

In any case a protected destructor sounds OK to me + scoped_ptr<WebVideoFrameImpl> inside webmediaplayer_impl.cc

[James Robinson]

Comment on attachment 95358 [details]
Patch

Err sorry, missed the discussion.  This shouldn't be public.

[Nico Weber]

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

[Darin Fisher (:fishd, Google)]

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

[Nico Weber]

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.

[Nico Weber]

Comment on attachment 95358 [details]
Patch

(see my last comment)

[Nico Weber]

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.

[Andrew Scherkus]

Yup I don't really care.

Code will likely change at some point anyway.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 95358 [details]:

http/tests/websocket/tests/error-detect.html bug 54012 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 95358 [details]
Patch

Clearing flags on attachment: 95358

Committed r88170: <http://trac.webkit.org/changeset/88170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Fisher (:fishd, Google)]

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

[Nico Weber]

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

[Andrew Scherkus]

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.