Bug 59470 – Prohibit the use of OwnPtr<T*>

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

[Adrienne Walker]

Prohibit the use of OwnPtr<T*>

[Adrienne Walker]

Created attachment 91129 [details]
Patch

[Adrienne Walker]

There may be some better way to implement this, but I think it would be an excellent idea to prevent somebody accidentally using OwnPtr<T*> when they intended OwnPtr<T>.  I can't think of any usage for the former.

(Also, I'm a little unsure of who to CC on this bug, so please add others.)

[WebKit Review Bot]

Attachment 91129 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8509701

[Build Bot]

Attachment 91129 [details] did not build on win:
Build output: http://queues.webkit.org/results/8508728

[WebKit Review Bot]

Attachment 91129 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8505948

[Adrienne Walker]

Created attachment 91144 [details]
Patch

[Adrienne Walker]

Let's try this again now that the blocking bugfix has landed.

[WebKit Review Bot]

Attachment 91144 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8509721

[Adrienne Walker]

Comment on attachment 91144 [details]
Patch

Oops.  Need to touch up DRT as well.

[WebKit Review Bot]

Attachment 91129 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8510701

[Build Bot]

Attachment 91144 [details] did not build on win:
Build output: http://queues.webkit.org/results/8509734

[WebKit Review Bot]

Attachment 91144 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8508780

[Adrienne Walker]

Created attachment 91177 [details]
Patch

[Adrienne Walker]

Third time's a charm for fixing all the build errors.

[Geoffrey Garen]

Comment on attachment 91177 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91177&action=review

r=me

> Source/JavaScriptCore/wtf/OwnPtr.h:179
> +    template<typename T> class OwnPtr<T*> {
> +        // Prevent usage as a real OwnPtr by not defining any functions.
> +    };
> +#endif

I believe you can do even better here by just using a declaration with no definition.

[Adrienne Walker]

(In reply to comment #15)
> (From update of attachment 91177 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=91177&action=review
> 
> r=me

Thanks for all the reviews today.  :)

> > Source/JavaScriptCore/wtf/OwnPtr.h:179
> > +    template<typename T> class OwnPtr<T*> {
> > +        // Prevent usage as a real OwnPtr by not defining any functions.
> > +    };
> > +#endif
> 
> I believe you can do even better here by just using a declaration with no definition.

That would be cleaner.  I'll do that before landing.

[Build Bot]

Attachment 91177 [details] did not build on win:
Build output: http://queues.webkit.org/results/8507775

[Adrienne Walker]

(In reply to comment #16)
> (In reply to comment #15)

> > > Source/JavaScriptCore/wtf/OwnPtr.h:179
> > > +    template<typename T> class OwnPtr<T*> {
> > > +        // Prevent usage as a real OwnPtr by not defining any functions.
> > > +    };
> > > +#endif
> > 
> > I believe you can do even better here by just using a declaration with no definition.
> 
> That would be cleaner.  I'll do that before landing.

After some study, I think an empty class rather than an incomplete declaration is better.

If the class is incomplete, then it's harder to #ifdef out this error because every class that is-a or has-a class with an OwnPtr<T*> needs the ALLOW_RAW_POINTER_IN_OWNPTR define.  Hopefully that should go away soon, but in the short term MediaPlayer.h is used in a number of places.

Also, the error message points to OwnPtr.h in the former case where somebody might see the comment, but just says the slightly more obscure "incomplete declaration" in the latter.

[WebKit Review Bot]

Attachment 91144 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/8510781

[Adrienne Walker]

Oh, yuck.  It looks like the Windows port is set up to handle OwnPtr<HBITMAP> (which is a void*), but has a deleteOwnPtr(HBITMAP) function which will properly clean up its resources.

So, I can't disallow all OwnPtr<T*>, just all except any except the 10 or so port-specific pointer types listed in OwnPtrCommon.h.

I'll have to take another look at this patch to account for that while still disallowing the general case.

[Darin Adler]

Comment on attachment 91177 [details]
Patch

Clearing review flag because we need to rework this for Windows’ sake. Best possibility would probably be to change Windows to use to something other than OwnPtr. It’s not safe to have it using OwnPtr with void*!

[Adrienne Walker]

(In reply to comment #21)
> (From update of attachment 91177 [details])
> Clearing review flag because we need to rework this for Windows’ sake. Best possibility would probably be to change Windows to use to something other than OwnPtr. It’s not safe to have it using OwnPtr with void*!

Ack.  I thought I had closed this bug.

It turns out that I was just mistaken.  All the Windows-specific versions of OwnPtr like OwnPtr<HBITMAP> have their own specialized delete function which gets called.

And thanks to template magic, for the general case of T, it turns out that OwnPtr<T*> is identical to OwnPtr<T> in terms of what gets stored in the OwnPtr and which delete function gets called.