Delete MutableHandle(nullptr_t) to prevent accidents

RESOLVED FIXED in mozilla27

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 806951 [details] [diff] [review]
disallow_null_mutablehandle-v0.diff

If someone accidentally passes NULL/nullptr to a MutableHandle argument, C++ will use the implicit constructor taking Rooted* -- just as it does for &rooted -- with dire consequences. With C++11, on supporting compilers, we can disallow this implicit constructor when null is passed.

I could have sworn that Luke filed this exact bug a few months ago, but, for the life of me, I cannot find it now.
Attachment #806951 - Flags: review?(jcoppeard)

Comment 1

5 years ago
Comment on attachment 806951 [details] [diff] [review]
disallow_null_mutablehandle-v0.diff

https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code claims you should use decltype(nullptr) instead of std::nullptr_t.  This would also match with comments by <http://hg.mozilla.org/mozilla-central/annotate/e8bb95435a54/xpcom/glue/nsCOMPtr.h#l150>.
I think a private `MutableHandle(int)` constructor would catch NULL and 0 argument bugs for all versions of C++.
(In reply to Chris Peterson (:cpeterson) from comment #2)

Having a private MutableHandle(int) seems to catch NULL/0 but let you pass nullptr.  Of course, we could have both.
Comment on attachment 806951 [details] [diff] [review]
disallow_null_mutablehandle-v0.diff

Review of attachment 806951 [details] [diff] [review]:
-----------------------------------------------------------------

I guess we should use decltype(nullptr) as suggested (is nullptr_t missing from the standard library on some compilers?  The comment in nsCOMPtr.h didn't make much sense to me).
Attachment #806951 - Flags: review?(jcoppeard) → review+
(Assignee)

Comment 5

5 years ago
(In reply to Jon Coppeard (:jonco) from comment #4)
> I guess we should use decltype(nullptr) as suggested (is nullptr_t missing
> from the standard library on some compilers?

That is correct, although I don't remember which platform.

(In reply to Chris Peterson (:cpeterson) from comment #2)
> I think a private `MutableHandle(int)` constructor would catch NULL and 0
> argument bugs for all versions of C++.

Nice! Added.

Green try at:
https://tbpl.mozilla.org/?tree=Try&rev=0ab99154e766

Landed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78c06cf60908

Comment 6

5 years ago
https://hg.mozilla.org/mozilla-central/rev/78c06cf60908
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.