Closed Bug 1025173 Opened 6 years ago Closed 6 years ago

Nullable copy, move, and assignment should not transfer |mValue| if |mIsNull|

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: erahm, Assigned: nyee, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1221255][lang=c++])

Attachments

(1 file, 3 obsolete files)

Nullable types are copying their internal value on assignment, copy, and move even if the Nullable is null (and the internal value is invalid/uninitialized/etc).

It's pretty easy to think up situations where this is a bad thing (or just inefficient), and it's making coverity unhappy.
Whiteboard: [CID 1221255] → [CID 1221255][mentor=bzbarsky@mit.edu][lang=c++]
I would like to take a stab at this. Which part of the codebase is relevant here?
Hello, Yati! Welcome aboard. bz, can you drop a DXR link in here for Yati to get started on?
Flags: needinfo?(bzbarsky)
Argh.  I keep failing to cc myself.

Yati, the relevant file is dom/bindings/Nullable.h, which you can read over at http://dxr.mozilla.org/mozilla-central/source/dom/bindings/Nullable.h
Flags: needinfo?(bzbarsky)
Mentor: bzbarsky
Whiteboard: [CID 1221255][mentor=bzbarsky@mit.edu][lang=c++] → [CID 1221255][lang=c++]
Attached patch bug-1025173-fix (obsolete) — Splinter Review
This is a partial patch, since I ran into template errors and don't know how to proceed. Any help with this bug would be greatly appreciated.
Attachment #8458444 - Flags: review?(bzbarsky)
Comment on attachment 8458444 [details] [diff] [review]
bug-1025173-fix

What template errors did you get?  Presumably from the move ctor?
Flags: needinfo?(ny.nathan.yee)
(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8458444 [details] [diff] [review]
> bug-1025173-fix
> 
> What template errors did you get?  Presumably from the move ctor?

I got the following errors:

Move ctor:
../../../dist/include/mozilla/dom/Nullable.h:43:14: error: use of deleted function ‘mozilla::dom::TypedArray<unsigned char, js::UnwrapArrayBuffer, JS_GetArrayBufferData, js::GetArrayBufferLengthAndData, JS_NewArrayBuffer>& mozilla::dom::TypedArray<unsigned char, js::UnwrapArrayBuffer, JS_GetArrayBufferData, js::GetArrayBufferLengthAndData, JS_NewArrayBuffer>::operator=(const mozilla::dom::TypedArray<unsigned char, js::UnwrapArrayBuffer, JS_GetArrayBufferData, js::GetArrayBufferLengthAndData, JS_NewArrayBuffer>&)’
        mValue = mozilla::Move(aOther.mValue);
               ^

Assignment ctor:
../../../dist/include/mozilla/dom/Nullable.h:43:14: error: no match for ‘operator=’ (operand types are ‘mozilla::dom::MozMap<JS::Value>’ and ‘mozilla::RemoveReference<mozilla::dom::MozMap<JS::Value>&>::Type {aka mozilla::dom::MozMap<JS::Value>}’)
        mValue = mozilla::Move(aOther.mValue);
               ^
So yes, from the move ctor.

It sounds like we should add move assignment operators (see <http://en.cppreference.com/w/cpp/language/move_operator>) to the TypedArray structs and MozMap.  But it worries me that those were the only errors.  Looking at that page, move assignment will fall back to copy assignment (as you can tell from the error messages above), so if things have copy assignment operators we're falling back on them here.  We could audit for bindings things that have move constructors but no move assignment operator, but it'd be a pain (e.g. nsBaseHashtable would also need to grow a move assignment operator).

Jeff, is there a principled way to catch such issues?

If not, we may end up having to use a Maybe for the mValue, which would add extra branches and the like.  At that point, I'm not sure it's worth worrying about this: looking back at comment 0, I can't think of situations where the copy is a bad thing, the inefficiency seems no worse than the null-checks in the common cases, and I'm not sure I care about Coverity all that much...
I get that these are outliers, but as-is Nullable's design allows for them.

#1 - Oversimplified pseudo-code example of copying being way more inefficient than a null-check:

Nullable<Array> Function() {
  Nullable<Array> result;
  result.SetValue().Reserve(100);
  if (!DoSomething(result))
     result.SetNull();
  return result; // as-is, the full array would be copied
}

#2 - Contrived example of being bad, but I think it gets the point across:

struct Foo {
  Foo() : bar(nullptr) {}
  Foo(Foo& foo) : bar(strdup(foo.mBar)) {}
  char* bar;
}

Nullable<Foo> Function() {
  Nullable<Foo> result;
  result.SetValue(new char[100]);
  if (!DoSomething(result)) {
     delete [] result.Value();
     result.SetNull();
  } 
  return result; // as-is, the full array would be copied
}
There are no such Foo used in bindings, so example 2 would not arise.

Example 1 _could_ arise with a nullable sequence.

Really, I'd be happy to just use Maybe inside Nullable and drop the separate mIsNull boolean, except that the members of Maybe are in the wrong order for that.
Speaking of using Maybe, is bug 913586 relevant to that consideration?
Only if they reorder the members and promise to keep them reordered....

But yes, we'd need it to support move constructors, of course.
Has anyone determined what should be done regarding this bug?
Flags: needinfo?(erahm)
Flags: needinfo?(ny.nathan.yee)
I've asked about getting Maybe<T> inline with what we're looking for in bug 913586. Let's see if that's an option.
Flags: needinfo?(erahm)
Comment on attachment 8458444 [details] [diff] [review]
bug-1025173-fix

I don't think this is correct unless we add move assignment operators to everything with a move constructor, sorry.
Attachment #8458444 - Flags: review?(bzbarsky) → review-
It looks like Maybe<T> landed (bug 913586), bz what would Nathan's next steps be?
Flags: needinfo?(bzbarsky)
Assuming Maybe now has the boolean first, the next step is probably to use a Maybe<T> member inside Nullable<T> instead of the boolean-and-T it has right now.  At that point you can have the copy/move/assignment delegate to the corresponding operations on Maybe<T> and things should just work.

We'll probably want to move the "mIsNull MUST COME FIRST" comment into the Maybe code.
Flags: needinfo?(bzbarsky)
Should I start with the instructions in Comment 16?
How do you include Maybe.h into Nullable.h?
#include "mozilla/Maybe.h"
Attached patch bug-1025173-fix [v.2] (Partial) (obsolete) — Splinter Review
Compiling this code gives "error: 'Mozilla' has not been declared" in
"mValue(Mozilla::Move(aOther.value));"
"       ^                            "
along with other errors.
Attachment #8458444 - Attachment is obsolete: true
Attachment #8480293 - Flags: review?(bzbarsky)
The mozilla namespace is lowercase.
Comment on attachment 8480293 [details] [diff] [review]
bug-1025173-fix [v.2] (Partial)

>+    : mValue(aValue)

mValue is now a Maybe<T>, not a T, right?  So you'll need to default-construct mValue and then mValue.emplace(aValue).

>+    if (aOther.mValue.isSome()) {
>+      mValue(Mozilla::Move(aOther.mValue));

The whole point of using Maybe is you don't have to check isSome() in cases like this.  It will handle that.  So you can leave this as an initializer like it was.

Same for the copy constructor.

Also, the assignment operator can keep using mValue's assignment operator; it will again do the right thing in the !isSome() case.

>   T& SetValue() {
>-    mIsNull = false;
>     return mValue;

No, this is wrong.  There was no need to do anything special with mValue here, since it was always initialized.  But now you have to check mValue.isNothing() and if so emplace() it.  Then return mValue.ref().

>   const T& Value() const {

Again, mValue is now a Maybe type.  So you want to return mValue.ref() here.  That will incidentally assert isSome() for you, so no need to do it yourself.

>   bool IsNull() const {

Should return mValue.isNothing().

>   bool Equals(const Nullable<T>& aOtherNullable) const

Should just return mValue == aOtherNullable.mValue, no?

I expect clearly thinking through the implications of mValue now no longer having type T should fix the compilation errors.
Attachment #8480293 - Flags: review?(bzbarsky)
I want to confirm whether "Mozilla::Move(aOther.mValue)" should be "mozilla::Move(aOther.mValue)".
I got an error when changing "const T& Value() const {}" to return "mValue.ref":

../../dist/include/mozilla/dom/Nullable.h: In instantiation of ‘const T& mozilla::dom::Nullable<T>::Value() const [with T = mozilla::TimeDuration]’:
../mozilla-central/dom/animation/Animation.cpp:109:52:   required from here
../../dist/include/mozilla/dom/Nullable.h:74:19: error: invalid initialization of reference of type ‘const mozilla::TimeDuration&’ from expression of type ‘<unresolved overloaded function type>’
       return mValue.ref;
                     ^

Is there anything I should do about this?
> should be "mozilla::Move(aOther.mValue)".

Yes, of course.  That's what it is in the code right now, no?

> Is there anything I should do about this?

Yes.  Return the value the function returns, not the function itself.  As in, "return mValue.ref();" like I said in comment 23.  ;)
Attached patch bug-1025173-fix [v.3] (Partial) (obsolete) — Splinter Review
This should be correct. What should we do about the mIsNull comment?
Attachment #8480293 - Attachment is obsolete: true
Attachment #8480962 - Flags: review?(bzbarsky)
Comment on attachment 8480962 [details] [diff] [review]
bug-1025173-fix [v.3] (Partial)

>   T& Value() {
>+    MOZ_ASSERT(mValue.isSome());

No need.  The .ref() call will handle it.

As far as the comment goes...  How about we remove the two conversion operators for Nullable<nsTArray>, and if that compiles we can just remove the comment.  If the need comes up for doing that again, we can worry about it at that point.

r=me with that, again assuming it compiles.
Attachment #8480962 - Flags: review?(bzbarsky) → review+
This compiles successfully. Any other nits in the code?
Attachment #8480962 - Attachment is obsolete: true
Attachment #8481079 - Flags: review?(bzbarsky)
Comment on attachment 8481079 [details] [diff] [review]
bug-1025173-fix [v.4]

Looks great, thanks!
Attachment #8481079 - Flags: review?(bzbarsky) → review+
I pushed https://tbpl.mozilla.org/?tree=Try&rev=386086f64485 and if that comes back green we should flag this as checkin-needed.
https://hg.mozilla.org/mozilla-central/rev/764e8c5fe598
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Duplicate of this bug: 1020414
Nathan, thanks again for working on this!
You need to log in before you can comment on or make changes to this bug.