883360 - Prevent bogus results from NS_ENSURE_TRUE(somePtr) when using a nsMainThreadPtrHandle

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Matthews [:jdm]]

Existing code like
>NS_ENSURE_TRUE(somePtr, NS_ERROR_FAILURE)
can silently break when changing somePtr to an nsMainThreadPtrHandle (such that you need somePtr.get() instead). It would be nice for this to Just Work.

Comment 1

[(no longer active)]

Making this "Just Work" will make it possible to write code which calls the implicit bool conversion operator without the author's knowledge, and lead into weird bugs creeping in.  I recommend against adding this bool conversion operator.

Comment 2

[Bobby Holley (:bholley)]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> Making this "Just Work" will make it possible to write code which calls the
> implicit bool conversion operator without the author's knowledge, and lead
> into weird bugs creeping in.

What kind of weird bugs, exactly?

Comment 3

[Josh Matthews [:jdm]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> Making this "Just Work" will make it possible to write code which calls the
> implicit bool conversion operator without the author's knowledge, and lead
> into weird bugs creeping in.  I recommend against adding this bool
> conversion operator.

On the other hand, it's really weird to see a smart pointer type containing null that is truthy for the purposes of snippets like comment 0.

Comment 4

[(no longer active)]

(In reply to comment #2)
> (In reply to :Ehsan Akhgari (needinfo? me!) from comment #1)
> > Making this "Just Work" will make it possible to write code which calls the
> > implicit bool conversion operator without the author's knowledge, and lead
> > into weird bugs creeping in.
> 
> What kind of weird bugs, exactly?

http://www.artima.com/cppsource/safebool.html

Comment 5

[Arnaud Sourioux [:Six]]

If i have well understood the safebool idiom it looks like we won't be able to keep nsMainThreadPtrHandle::operator== if we want to use safebool

From the previous link about operator== : 
"I've added an (parameterized) implementation that fails to compile (when instantiated)"

because this can lead to call the specific implementation of "operator bool()" on both objects instead of really calling operator==

Correct me if i'm wrong :)

Does this ticket still worth it? (as it might need many changes, everywhere nsMainThreadPtrHandle::operator== is called)

Comment 6

[(no longer active)]

I'm not sure if I understand what comment 5 is talking about.  Can you please show a patch and some code that doesn't compile as a result of that patch?

Comment 7

[Arnaud Sourioux [:Six]]

Attachment: add operator bool() to nsMainThreadPtrHandle using SafeBool Idiom

Feature has been implemented using ehsan link about SafeBool Idiom
Josh, does it looks like what you are needing?

Comment 8

[Benjamin Smedberg]

It seems clear to me that NS_ENSURE_TRUE(somePtr) should *either* return the correct thing or be a compile-time failure. If I'm understanding the issue correctly, nsMainThreadPtrHandle will right now auto-bool using `operator nsMainThreadPtrHolder<T>*` which doesn't test the nullness of the underlying pointer. But in that case, we probably ought to be explicit about it, and should remove the operator so that the construct fails to compile entirely.

Comment 9

[Benjamin Smedberg]

Comment on attachment 766079 [details] [diff] [review]
add operator bool() to nsMainThreadPtrHandle using SafeBool Idiom

Going to mark r- based on that feedback; please re-request review if I missed some important piece of information.

Comment 10

[Josh Matthews [:jdm]]

That reasoning sounds pretty solid to me.

Comment 11

[Arnaud Sourioux [:Six]]

Attachment: add operator bool() to nsMainThreadPtrHandle using SafeBool Idiom with last comments

The same patch but with last comments.

I had to add some new ctor to classes that were taking nsMainThreadPtrHolder as parameter, it build correctly on my linux64b

i also had to comment 
> nsMainThreadPtrHandle::operator bool() { return get(); } 
wich has been added by bug 497003

Comment 12

[Arnaud Sourioux [:Six]]

Attachment: add operator bool() to nsMainThreadPtrHandle using SafeBool Idiom with last comments

The same patch but with last comments.

I had to add some new ctor to classes that were taking nsMainThreadPtrHolder as parameter, it build correctly on my linux64b

i also had the same patch but with last comments.

I had to add some new ctor to classes that were taking nsMainThreadPtrHolder as parameter, it build correctly on my linux64b

i also had to comment 
> nsMainThreadPtrHandle::operator bool() { return get(); } 
wich has been added by bug 497003

Comment 13

[Benjamin Smedberg]

Comment on attachment 775328 [details] [diff] [review]
add operator bool() to nsMainThreadPtrHandle using SafeBool Idiom with last comments

What is the point of this patch now? Can we avoid all of the SafeBool stuff here?

Comment 14

[Arnaud Sourioux [:Six]]

Attachment: Remove automatic bool conversion operator

I added some new ctor to classes that were taking nsMainThreadPtrHolder as parameter, it build correctly on my linux64b

i also had to remove
> nsMainThreadPtrHandle::operator bool()
wich has been added by bug 497003

Comment 15

[Benjamin Smedberg]

Comment on attachment 775861 [details] [diff] [review]
Remove automatic bool conversion operator

I believe that to avoid extra refcounting, you really want these to be a refrence or a const reference instead of a bare class, that is:

    OnSocketAcceptedRunnable(const nsMainThreadPtrHandle<nsIServerSocketListener>& aListener,
                             nsIServerSocket* aServ,
                             nsISocketTransport* aTransport)

In many of these cases, though, I'm not convinced that the extra constructor is worth it. Can't you just use .get() for these?

Comment 16

[Arnaud Sourioux [:Six]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #15)
> In many of these cases, though, I'm not convinced that the extra constructor
> is worth it. Can't you just use .get() for these?

jduell/mcmanus:
what do you prefer?
as this patch will remove the 'operator nsMainThreadPtrHolder<T>*' we have thoses two choices to make it work:

- add a new Ctor with const nsMainThreadPtrHandle<nsIServerSocketListener>& as parameter
- use nsMainThreadPtrHolder::get() when we call the actual Ctor

in classes like OnSocketAcceptedRunnable, LookupCompleteRunnable, StreamFinishedRunnable and others

Comment 17

[Jason Duell]

as discussed on IRC we'll go with the new Ctor.

Comment 18

[Arnaud Sourioux [:Six]]

Attachment: Remove bool operator and nsMainThreadPtrHolder conversion

This patch is what was wanted
tbpl is green: https://tbpl.mozilla.org/?tree=Try&rev=f6c7cc89ee14

Comment 19

[Josh Matthews [:jdm]]

Comment on attachment 778138 [details] [diff] [review]
Remove bool operator and nsMainThreadPtrHolder conversion

Are all the duplicated constructors necessary? Can't you remove all the original ones?

Comment 20

[Arnaud Sourioux [:Six]]

Attachment: Remove bool operator and nsMainThreadPtrHolder conversion. modify impacted Ctor's

As Jdm said, we can avoid to have unused Ctor in thoses classes.
tpbl was full green: https://tbpl.mozilla.org/?tree=Try&rev=3d32958b47c5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7900b47971c7

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7900b47971c7