865240 - fix IUnknown implementation

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, major)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Since mRefCnt is not initialized for NS_DECL_IUNKNOWN classes then it leads to crashes when calling a method. For example, because of that JAWS loose virtual buffer occasionally. sdnSimpleDOMNode, serviceProvider, ia2 classes are affected. 

Btw, I think bug 859781 is caused by described behavior.

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

more cleaning-ups

Comment 2

[alexander :surkov (:asurkov)]

JAWS behavior is regression from bug 767756, but this bug has a longer story

Comment 3

[Trevor Saunders (:tbsaunde)]

Comment on attachment 741674 [details] [diff] [review]
patch2

>-    *ppv = new sdnAccessible(GetNode());
>+    *ppv = static_cast<ISimpleDOMNode*>(new sdnAccessible(GetNode()));

I'm pretty sure you don't need these casts

>-
>+#include "IUnknownImpl.h"

does AccessibleWrap.h actually depend on this for something? I don't see what it is

>--- a/accessible/src/windows/msaa/AccessibleWrap.h
>+++ b/accessible/src/windows/msaa/IUnknownImpl.h
>@@ -1,57 +1,77 @@
> /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set ts=2 et sw=2 tw=80: */
> /* This Source Code Form is subject to the terms of the Mozilla Public
>  * License, v. 2.0. If a copy of the MPL was not distributed with this
>- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>-
>-/* For documentation of the accessibility architecture, 
>- * see http://lxr.mozilla.org/seamonkey/source/accessible/accessible-docs.html
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. *
>  */

comment looks funny are you sure its right format?

>+#include "Unknwn.h"

what do you need it for?

>+
>+class AutoRefCnt

name space it?

>+  AutoRefCnt(ULONG aValue) : mValue(aValue) {}

what do you need this constructor for?

>+  // only support prefix increment/decrement
>+  ULONG operator++() { return ++mValue; }
>+  ULONG operator--() { return --mValue; }
>+
>+  ULONG operator=(ULONG aValue) { return (mValue = aValue); }

why do you need this?

>+  ULONG get() const { return mValue; }

and this

>+
>+private:
>+  // do not define these to enforce the faster prefix notation
>+  ULONG operator++(int);

that's silly, it's no faster with recent compilers.  If you actually want to force prefix then you should MOZ_DELETE these though, but I'd prefer you just defined the postfix ones and left the other code as is.

> #define DECL_IUNKNOWN                                                          \
> public:                                                                        \
>   virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID, void**);            \
>   virtual ULONG STDMETHODCALLTYPE AddRef() MOZ_FINAL                           \
>-    {  return ++mRefCnt; }                                                     \
>+  {                                                                            \
>+    MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                       \

not actually illegal, but pretty crazy so I guess its ok

>                                                                                \
>+     mRefCnt = 1; /* stabilize */                                              \

do you actually need that for something?  We shouldn't ever hold onto these objects so should never ajust their refcount especially during dtor so it shouldn't matter

> EXPORTS += [
>+    'IUnknownImpl.h',

why do you need this?

>+++ b/accessible/src/windows/sdn/sdnAccessible.cpp
>@@ -29,29 +29,29 @@ sdnAccessible::QueryInterface(REFIID aRE
> {
>   A11Y_TRYBLOCK_BEGIN
> 
>   if (!aInstancePtr)
>     return E_FAIL;
>   *aInstancePtr = nullptr;
> 
>   if (aREFIID == IID_ISimpleDOMNode) {
>-    *aInstancePtr = this;
>+    *aInstancePtr = static_cast<ISimpleDOMNode*>(this);

pretty sure you don't need these casts either

do you want to write a minimal patch for branches or shall I?

Comment 4

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #3)
> Comment on attachment 741674 [details] [diff] [review]
> patch2
> 
> >-    *ppv = new sdnAccessible(GetNode());
> >+    *ppv = static_cast<ISimpleDOMNode*>(new sdnAccessible(GetNode()));
> 
> I'm pretty sure you don't need these casts

I recall we had problems in the past because of missed casts. Also we used to have them in our QI. I just get them back to stay consistent.

> >-
> >+#include "IUnknownImpl.h"
> 
> does AccessibleWrap.h actually depend on this for something? I don't see
> what it is

no need I guess

> >--- a/accessible/src/windows/msaa/AccessibleWrap.h
> >+++ b/accessible/src/windows/msaa/IUnknownImpl.h
> >@@ -1,57 +1,77 @@
> > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> >+/* vim: set ts=2 et sw=2 tw=80: */
> > /* This Source Code Form is subject to the terms of the Mozilla Public
> >  * License, v. 2.0. If a copy of the MPL was not distributed with this
> >- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >-
> >-/* For documentation of the accessibility architecture, 
> >- * see http://lxr.mozilla.org/seamonkey/source/accessible/accessible-docs.html
> >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. *
> >  */
> 
> comment looks funny are you sure its right format?

what's funny about it, it seems the same as http://www.mozilla.org/MPL/headers/

> >+#include "Unknwn.h"
> 
> what do you need it for?

IUnknown definition

> >+
> >+class AutoRefCnt
> 
> name space it?

ok, putting into mozilla::a11y

> >+  AutoRefCnt(ULONG aValue) : mValue(aValue) {}
> 
> what do you need this constructor for?

copy/paste issue

> >+  // only support prefix increment/decrement
> >+  ULONG operator++() { return ++mValue; }
> >+  ULONG operator--() { return --mValue; }
> >+
> >+  ULONG operator=(ULONG aValue) { return (mValue = aValue); }
> 
> why do you need this?

to stabilize it

> >+  ULONG get() const { return mValue; }
> 
> and this

copy/paste

> >+
> >+private:
> >+  // do not define these to enforce the faster prefix notation
> >+  ULONG operator++(int);
> 
> that's silly, it's no faster with recent compilers.  If you actually want to
> force prefix then you should MOZ_DELETE these though, but I'd prefer you
> just defined the postfix ones and left the other code as is.

ok

> > #define DECL_IUNKNOWN                                                          \
> > public:                                                                        \
> >   virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID, void**);            \
> >   virtual ULONG STDMETHODCALLTYPE AddRef() MOZ_FINAL                           \
> >-    {  return ++mRefCnt; }                                                     \
> >+  {                                                                            \
> >+    MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                       \
> 
> not actually illegal, but pretty crazy so I guess its ok
> 
> >                                                                                \
> >+     mRefCnt = 1; /* stabilize */                                              \
> 
> do you actually need that for something?  We shouldn't ever hold onto these
> objects so should never ajust their refcount especially during dtor so it
> shouldn't matter

We don't need this but I think it's general pattern to avoid double deletion if somebody addrefs/release the object in its desctructor. Again I just copied/pasted it form xpcom version.

> > EXPORTS += [
> >+    'IUnknownImpl.h',
> 
> why do you need this?

for example, it's needed for HyperTextAccessibleWrap

> >+++ b/accessible/src/windows/sdn/sdnAccessible.cpp
> >@@ -29,29 +29,29 @@ sdnAccessible::QueryInterface(REFIID aRE
> > {
> >   A11Y_TRYBLOCK_BEGIN
> > 
> >   if (!aInstancePtr)
> >     return E_FAIL;
> >   *aInstancePtr = nullptr;
> > 
> >   if (aREFIID == IID_ISimpleDOMNode) {
> >-    *aInstancePtr = this;
> >+    *aInstancePtr = static_cast<ISimpleDOMNode*>(this);
> 
> pretty sure you don't need these casts either
> 
> do you want to write a minimal patch for branches or shall I?

Comment 5

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 6

[Trevor Saunders (:tbsaunde)]

> > >--- a/accessible/src/windows/msaa/AccessibleWrap.h
> > >+++ b/accessible/src/windows/msaa/IUnknownImpl.h
> > >@@ -1,57 +1,77 @@
> > > /* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > >+/* vim: set ts=2 et sw=2 tw=80: */
> > > /* This Source Code Form is subject to the terms of the Mozilla Public
> > >  * License, v. 2.0. If a copy of the MPL was not distributed with this
> > >- * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > >-
> > >-/* For documentation of the accessibility architecture, 
> > >- * see http://lxr.mozilla.org/seamonkey/source/accessible/accessible-docs.html
> > >+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. *
> > >  */
> > 
> > comment looks funny are you sure its right format?
> 
> what's funny about it, it seems the same as
> http://www.mozilla.org/MPL/headers/

I thought */ was suposed to be on previous line

> > >+#include "Unknwn.h"
> > 
> > what do you need it for?
> 
> IUnknown definition

does this file actually need that for something?  I guess it needs hresult and stuff though.

> > >+  // only support prefix increment/decrement
> > >+  ULONG operator++() { return ++mValue; }
> > >+  ULONG operator--() { return --mValue; }
> > >+
> > >+  ULONG operator=(ULONG aValue) { return (mValue = aValue); }
> > 
> > why do you need this?
> 
> to stabilize it

my point was we shouldn't need to do that

> > > #define DECL_IUNKNOWN                                                          \
> > > public:                                                                        \
> > >   virtual HRESULT STDMETHODCALLTYPE QueryInterface(REFIID, void**);            \
> > >   virtual ULONG STDMETHODCALLTYPE AddRef() MOZ_FINAL                           \
> > >-    {  return ++mRefCnt; }                                                     \
> > >+  {                                                                            \
> > >+    MOZ_ASSERT(int32_t(mRefCnt) >= 0, "illegal refcnt");                       \
> > 
> > not actually illegal, but pretty crazy so I guess its ok
> > 
> > >                                                                                \
> > >+     mRefCnt = 1; /* stabilize */                                              \
> > 
> > do you actually need that for something?  We shouldn't ever hold onto these
> > objects so should never ajust their refcount especially during dtor so it
> > shouldn't matter
> 
> We don't need this but I think it's general pattern to avoid double deletion
> if somebody addrefs/release the object in its desctructor. Again I just
> copied/pasted it form xpcom version.

we really shouldn't have that problem with any of the things we use it for

> > > EXPORTS += [
> > >+    'IUnknownImpl.h',
> > 
> > why do you need this?
> 
> for example, it's needed for HyperTextAccessibleWrap

ok

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> > > >+#include "Unknwn.h"
> > > 
> > > what do you need it for?
> > 
> > IUnknown definition
> 
> does this file actually need that for something?  I guess it needs hresult
> and stuff though.

and IUnknown itself too

> > We don't need this but I think it's general pattern to avoid double deletion
> > if somebody addrefs/release the object in its desctructor. Again I just
> > copied/pasted it form xpcom version.
> 
> we really shouldn't have that problem with any of the things we use it for

if you insist then I will remove it, I just followed the existing pattern assuming it has a reason

Comment 8

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > > > >+#include "Unknwn.h"
> > > > 
> > > > what do you need it for?
> > > 
> > > IUnknown definition
> > 
> > does this file actually need that for something?  I guess it needs hresult
> > and stuff though.
> 
> and IUnknown itself too

oh?  Which of those macros needs it?  Though I guess all the places that include this header will probably pull it in other ways to so its probably not a big deal.

> > > We don't need this but I think it's general pattern to avoid double deletion
> > > if somebody addrefs/release the object in its desctructor. Again I just
> > > copied/pasted it form xpcom version.
> > 
> > we really shouldn't have that problem with any of the things we use it for
> 
> if you insist then I will remove it, I just followed the existing pattern
> assuming it has a reason

Well, I think it has a purpose in the xpcom case just not this one.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> > and IUnknown itself too
> 
> oh?  Which of those macros needs it?  Though I guess all the places that
> include this header will probably pull it in other ways to so its probably
> not a big deal.

ok :) just having macros for IUnknown requiring a separate IUnknown include looked strange.

> > > > We don't need this but I think it's general pattern to avoid double deletion
> > > > if somebody addrefs/release the object in its desctructor. Again I just
> > > > copied/pasted it form xpcom version.
> > > 
> > > we really shouldn't have that problem with any of the things we use it for
> > 
> > if you insist then I will remove it, I just followed the existing pattern
> > assuming it has a reason
> 
> Well, I think it has a purpose in the xpcom case just not this one.

ok, I will remove it

Comment 10

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #9)
> (In reply to Trevor Saunders (:tbsaunde) from comment #8)
> 
> > > and IUnknown itself too
> > 
> > oh?  Which of those macros needs it?  Though I guess all the places that
> > include this header will probably pull it in other ways to so its probably
> > not a big deal.
> 
> ok :) just having macros for IUnknown requiring a separate IUnknown include
> looked strange.

well, presumably your implementing IFoo so you'll be including ifoo.h or whatever which will pull in unknown.h

Comment 11

[Trevor Saunders (:tbsaunde)]

Comment on attachment 742777 [details] [diff] [review]
patch2

r=me with comments fixed

Comment 12

[Trevor Saunders (:tbsaunde)]

Attachment: bug 865240 - backport initializing mRefCnt in SDNAccessible

Alex would you mind checking this is the only case we missed?

Comment 13

[Trevor Saunders (:tbsaunde)]

Comment on attachment 743267 [details] [diff] [review]
bug 865240 - backport initializing mRefCnt in SDNAccessible

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 767756
User impact if declined: crashes
Testing completed (on m-c, etc.): not yet landed but the patch for current branche is larger and fairly different
Risk to taking this patch (and alternatives if risky): none, it just initializes a value instead of letting it be some psuedo random value.
String or IDL/UUID changes made by this patch:none

Comment 14

[Alex Keybl [:akeybl]]

Comment on attachment 743267 [details] [diff] [review]
bug 865240 - backport initializing mRefCnt in SDNAccessible

We'll approve for Aurora once this has r+, but since this regression is in FF20, there's little reason to react urgently in our last two FF21 betas.

Comment 15

[alexander :surkov (:asurkov)]

Comment on attachment 743267 [details] [diff] [review]
bug 865240 - backport initializing mRefCnt in SDNAccessible

thank you for doing it

Comment 16

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d6b37fd04df4

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Alex Keybl [:akeybl] from comment #14)
> Comment on attachment 743267 [details] [diff] [review]
> bug 865240 - backport initializing mRefCnt in SDNAccessible
> 
> We'll approve for Aurora once this has r+, but since this regression is in
> FF20, there's little reason to react urgently in our last two FF21 betas.

It's a serious regression for JAWS users, I believe it makes them to skip broken versions since many sites are intermittently inaccessible, see bug 859781 and bug 866495. If we have a chance to put a fix into Fx 21 then we should do it, especially because the fix is trivial.

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[bhavana bajaj [:bajaj]]

Comment on attachment 743267 [details] [diff] [review]
bug 865240 - backport initializing mRefCnt in SDNAccessible

Upon further context given in https://bugzilla.mozilla.org/show_bug.cgi?id=865240#c17, reconsidering our decision and approving on mozilla-beta.

Can we please get the needed testing on aurora/beta here to find out any new regression we should know considering this is our last beta .

Comment 20

[alexander :surkov (:asurkov)]

Marco, would you mind to land patches please and give it a testing?

Comment 21

[Everett Zufelt]

Tested in nightly 2013-05-01 and my initial problem is resolved.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8132646315dd
https://hg.mozilla.org/releases/mozilla-beta/rev/dc115ef09473

Comment 24

[alexander :surkov (:asurkov)]

Our AT partners confirmed the bug is fixed so we don't need QA on our side.

Comment 25

[Dan]

Thankyou for this fix. I use JAWS11 and with FF21 this crashing is completely gone now. This is a very important fix for me and probably many others -- thankyou!