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

Status: RESOLVED FIXED

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

Description

[Eric Rahm [:erahm]]

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.

Comment 1

[Yati Sagade]

I would like to take a stab at this. Which part of the codebase is relevant here?

Comment 2

[Mike Hoye [:mhoye]]

Hello, Yati! Welcome aboard. bz, can you drop a DXR link in here for Yati to get started on?

Comment 3

[Boris Zbarsky [: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

Comment 4

[Nathan Yee [:nyee]]

Attachment: bug-1025173-fix

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8458444 [details] [diff] [review]
bug-1025173-fix

What template errors did you get?  Presumably from the move ctor?

Comment 6

[Nathan Yee [:nyee]]

(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);
               ^

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

Comment 8

[Eric Rahm [:erahm]]

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
}

Comment 9

[Boris Zbarsky [:bzbarsky]]

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.

Comment 10

[Emanuel Hoogeveen [:ehoogeveen]]

Speaking of using Maybe, is bug 913586 relevant to that consideration?

Comment 11

[Boris Zbarsky [:bzbarsky]]

Only if they reorder the members and promise to keep them reordered....

But yes, we'd need it to support move constructors, of course.

Comment 12

[Nathan Yee [:nyee]]

Has anyone determined what should be done regarding this bug?

Comment 13

[Eric Rahm [:erahm]]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

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.

Comment 15

[Eric Rahm [:erahm]]

It looks like Maybe<T> landed (bug 913586), bz what would Nathan's next steps be?

Comment 16

[Boris Zbarsky [: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.

Comment 17

[Nathan Yee [:nyee]]

Should I start with the instructions in Comment 16?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Please!

Comment 19

[Nathan Yee [:nyee]]

How do you include Maybe.h into Nullable.h?

Comment 20

[Boris Zbarsky [:bzbarsky]]

#include "mozilla/Maybe.h"

Comment 21

[Nathan Yee [:nyee]]

Attachment: bug-1025173-fix [v.2] (Partial)

Compiling this code gives "error: 'Mozilla' has not been declared" in
"mValue(Mozilla::Move(aOther.value));"
"       ^                            "
along with other errors.

Comment 22

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

The mozilla namespace is lowercase.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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.

Comment 24

[Nathan Yee [:nyee]]

I want to confirm whether "Mozilla::Move(aOther.mValue)" should be "mozilla::Move(aOther.mValue)".

Comment 25

[Nathan Yee [:nyee]]

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?

Comment 26

[Boris Zbarsky [:bzbarsky]]

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

Comment 27

[Nathan Yee [:nyee]]

Attachment: bug-1025173-fix [v.3] (Partial)

This should be correct. What should we do about the mIsNull comment?

Comment 28

[Boris Zbarsky [: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.

Comment 29

[Nathan Yee [:nyee]]

Attachment: bug-1025173-fix [v.4]

This compiles successfully. Any other nits in the code?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8481079 [details] [diff] [review]
bug-1025173-fix [v.4]

Looks great, thanks!

Comment 31

[Boris Zbarsky [:bzbarsky]]

I pushed https://tbpl.mozilla.org/?tree=Try&rev=386086f64485 and if that comes back green we should flag this as checkin-needed.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/764e8c5fe598

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/764e8c5fe598

Comment 35

[Boris Zbarsky [:bzbarsky]]

Nathan, thanks again for working on this!