539694 - accessible objects should have private copy constructor

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Accessible objects should define copy constructor and assignment operators in private section to avoid accident calls (internal or on AT side). It was inspired by talk with Pete.

Comment 1

[alexander :surkov (:asurkov)]

check all classes in folder http://mxr.mozilla.org/mozilla-central/source/accessible/src/

Comment 2

[Trevor Saunders (:tbsaunde)]

it appears you only need to check nsAccessNode and  not any of the things that inherit from it because having a private copy constructor / operator = means  things inheriting from you can't copy you.  Here's a test with gcc 4.6

tbsaunde@football:/tmp$ cat test.cpp
class foo
{
  public:
    foo() { }
  private:
  foo& operator =(const foo& aValue);
  foo(const foo&);
};

class dir : public foo
{
  public:
  dir() : foo() { }
  int mType;
};

  int main()
{
  dir bar;
  dir baz = bar;
  return baz.mType;
}

tbsaunde@football:/tmp$ g++ test.cpp
test.cpp: In copy constructor ‘dir::dir(const dir&)’:
test.cpp:7:3: error: ‘foo::foo(const foo&)’ is private
test.cpp:10:7: error: within this context
test.cpp: In function ‘int main()’:
test.cpp:20:13: note: synthesized method ‘dir::dir(const dir&)’ first required here
tbsaunde@football:/tmp$

sorry about the weird characters in the error message, it would appear something between bugzilla and my terminal dislikes utf8 :/

Comment 3

[Jignesh Kakadiya [:jhk]]

Attachment: Introduced copy constructor

patch_1

Comment 4

[Jignesh Kakadiya [:jhk]]

Attachment: Patch_2

.

Comment 5

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581205 [details] [diff] [review]
Patch_2

r=tbsaunde not surkov

Comment 6

[Jignesh Kakadiya [:jhk]]

Attachment: Introduced copy constructor

r = tbsaunde

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 581211 [details] [diff] [review]
Introduced copy constructor

review not needed since same patch.

Comment 8

[Trevor Saunders (:tbsaunde)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/332cdb521dd6

THANKS!

Comment 9

[Trevor Saunders (:tbsaunde)]

actually, https://hg.mozilla.org/integration/mozilla-inbound/rev/54c446091d7c
 push race :/

Comment 10

[Jeff Walden [:Waldo]]

Just to note, there's a new macro you should use to do this, MOZ_DELETE:

http://hg.mozilla.org/mozilla-central/annotate/3c321d2c9884/mfbt/Attributes.h#l106

It's just a matter of #include "mozilla/Attributes.h", then putting MOZ_DELETE at the ends of declarations like the ones in comment 9's URL.  This has the advantage of producing errors at compile time, not (potentially) at link time (if the code making the accidental copy is a friend of the class being copied).

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/54c446091d7c

Leaving open for comment 10.

Comment 12

[alexander :surkov (:asurkov)]

Jignesh, could you address comment #10 please?

Comment 13

[Jignesh Kakadiya [:jhk]]

Attachment: Thanks for Pointing out.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 595446 [details] [diff] [review]
Thanks for Pointing out.

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

r=me, thank you for doing this

Comment 15

[alexander :surkov (:asurkov)]

inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/c6dda929759e

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c6dda929759e