743680 - don't export nsARIAMap.h

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch2

fix build issues

Comment 2

[Trevor Saunders (:tbsaunde)]

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

>diff --git a/accessible/src/base/nsAccUtils.cpp b/accessible/src/base/nsAccUtils.cpp
>--- a/accessible/src/base/nsAccUtils.cpp
>+++ b/accessible/src/base/nsAccUtils.cpp
> #include "nsARIAMap.h"
> #include "nsDocAccessible.h"
>+#include "nsCoreUtils.h"

shouldn't it come before nsDocAccessible.h?

>+  bool HasARIARole() const
>+    { return mRoleMapEntry; }

without checking the callers, wouldn't it make more sense for this to check mUseMappedRole here too?

>+#ifndef _nsAccessible_inl_H_
>+#define _nsAccessible_inl_H_

I gues that name sort of makes sense for now, but MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense.

> #import "mozView.h"
> #import "nsRoleMap.h"
> 
>+#include "Accessible-inl.h"
>+#include "nsIAccessible.h"

couldn't you get rid of that one?

> #import "mozActionElements.h"
> 
> #import "MacUtils.h"
>-
>-#import "nsIAccessible.h"
>+#import "nsAccessible-inl.h"

how it work?

> enum CheckboxValue {
>   // these constants correspond to the values in the OS
>diff --git a/accessible/src/mac/nsAccessibleWrap.mm b/accessible/src/mac/nsAccessibleWrap.mm
>--- a/accessible/src/mac/nsAccessibleWrap.mm
>+++ b/accessible/src/mac/nsAccessibleWrap.mm
>@@ -36,16 +36,17 @@
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsDocAccessible.h"
> #include "nsObjCExceptions.h"
> 
> #import "nsRoleMap.h"
> 
>+#include "Accessible-inl.h"
> #include "Role.h"
> 
> #import "mozAccessible.h"
> #import "mozActionElements.h"
> #import "mozHTMLAccessible.h"
> #import "mozTextAccessible.h"
> 
> using namespace mozilla::a11y;
>diff --git a/accessible/src/xul/nsXULColorPickerAccessible.cpp b/accessible/src/xul/nsXULColorPickerAccessible.cpp
>--- a/accessible/src/xul/nsXULColorPickerAccessible.cpp
>+++ b/accessible/src/xul/nsXULColorPickerAccessible.cpp
>@@ -33,16 +33,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXULColorPickerAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccUtils.h"
> #include "nsAccTreeWalker.h"
> #include "nsCoreUtils.h"
> #include "nsDocAccessible.h"
> #include "Role.h"
> #include "States.h"
> 
> #include "nsIDOMElement.h"
>diff --git a/accessible/src/xul/nsXULComboboxAccessible.cpp b/accessible/src/xul/nsXULComboboxAccessible.cpp
>--- a/accessible/src/xul/nsXULComboboxAccessible.cpp
>+++ b/accessible/src/xul/nsXULComboboxAccessible.cpp
>@@ -35,16 +35,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXULComboboxAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
> #include "nsDocAccessible.h"
> #include "nsCoreUtils.h"
> #include "Role.h"
> #include "States.h"
> 
> #include "nsIAutoCompleteInput.h"
> #include "nsIDOMXULMenuListElement.h"
>diff --git a/accessible/src/xul/nsXULFormControlAccessible.cpp b/accessible/src/xul/nsXULFormControlAccessible.cpp
>--- a/accessible/src/xul/nsXULFormControlAccessible.cpp
>+++ b/accessible/src/xul/nsXULFormControlAccessible.cpp
>@@ -34,16 +34,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXULFormControlAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccUtils.h"
> #include "nsAccTreeWalker.h"
> #include "nsCoreUtils.h"
> #include "nsDocAccessible.h"
> #include "Relation.h"
> #include "Role.h"
> #include "States.h"
> 
>diff --git a/accessible/src/xul/nsXULListboxAccessible.cpp b/accessible/src/xul/nsXULListboxAccessible.cpp
>--- a/accessible/src/xul/nsXULListboxAccessible.cpp
>+++ b/accessible/src/xul/nsXULListboxAccessible.cpp
>@@ -35,16 +35,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXULListboxAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
> #include "nsAccUtils.h"
> #include "nsDocAccessible.h"
> #include "Role.h"
> #include "States.h"
> 
> #include "nsComponentManagerUtils.h"
> #include "nsIAutoCompleteInput.h"
>diff --git a/accessible/src/xul/nsXULMenuAccessible.cpp b/accessible/src/xul/nsXULMenuAccessible.cpp
>--- a/accessible/src/xul/nsXULMenuAccessible.cpp
>+++ b/accessible/src/xul/nsXULMenuAccessible.cpp
>@@ -33,16 +33,17 @@
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #include "nsXULMenuAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
> #include "nsAccUtils.h"
> #include "nsDocAccessible.h"
> #include "nsXULFormControlAccessible.h"
> #include "Role.h"
> #include "States.h"
> 
> #include "nsIDOMElement.h"
>diff --git a/accessible/src/xul/nsXULTextAccessible.cpp b/accessible/src/xul/nsXULTextAccessible.cpp
>--- a/accessible/src/xul/nsXULTextAccessible.cpp
>+++ b/accessible/src/xul/nsXULTextAccessible.cpp
>@@ -35,16 +35,17 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> // NOTE: groups are alphabetically ordered
> #include "nsXULTextAccessible.h"
> 
>+#include "Accessible-inl.h"
> #include "nsAccUtils.h"
> #include "nsBaseWidgetAccessible.h"
> #include "nsCoreUtils.h"
> #include "nsTextEquivUtils.h"
> #include "Relation.h"
> #include "Role.h"
> #include "States.h"
>

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> >+  bool HasARIARole() const
> >+    { return mRoleMapEntry; }
> 
> without checking the callers, wouldn't it make more sense for this to check
> mUseMappedRole here too?

maybe, don't want to go into aria-activedescendant logic.

> >+#ifndef _nsAccessible_inl_H_
> >+#define _nsAccessible_inl_H_
> 
> I gues that name sort of makes sense for now, but
> MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense.

I can do that, btw, why do you like to keep them in capslock since it doens't look super nice. For example, Element.h does the same http://mxr.mozilla.org/mozilla-central/source/content/base/public/Element.h. I think I'd go with mozilla_a11y_Accessible_inl_h_

> >-#import "nsIAccessible.h"
> >+#import "nsAccessible-inl.h"
> 
> how it work?

#import and #include is mostly the same thing, #import makes sure to not include the file twice. I get back to #include for internal header file.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > >+  bool HasARIARole() const
> > >+    { return mRoleMapEntry; }
> > 
> > without checking the callers, wouldn't it make more sense for this to check
> > mUseMappedRole here too?
> 
> maybe, don't want to go into aria-activedescendant logic.

ok

> > >+#ifndef _nsAccessible_inl_H_
> > >+#define _nsAccessible_inl_H_
> > 
> > I gues that name sort of makes sense for now, but
> > MOZILLA_A11Y_ACCESSIBLE_INL_H_ seems like it makes more sense.
> 
> I can do that, btw, why do you like to keep them in capslock since it
> doens't look super nice. For example, Element.h does the same
> http://mxr.mozilla.org/mozilla-central/source/content/base/public/Element.h.
> I think I'd go with mozilla_a11y_Accessible_inl_h_

ok, I think its just habit to make macros all caps, but I don't think it really matters so long as its unique which I think it is either way.

> > >-#import "nsIAccessible.h"
> > >+#import "nsAccessible-inl.h"
> > 
> > how it work?
> 
> #import and #include is mostly the same thing, #import makes sure to not
> include the file twice. I get back to #include for internal header file.

sure, but "nsAccessible-inl.h"?

Comment 5

[alexander :surkov (:asurkov)]

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

> > #import and #include is mostly the same thing, #import makes sure to not
> > include the file twice. I get back to #include for internal header file.
> 
> sure, but "nsAccessible-inl.h"?

yep, I noticed that and fixed, it didn't work I assume.

Comment 6

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0975fb1e62d

Comment 7

[Marco Bonardo [:mak]]

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

Comment 8

[George Wright (:gw280) (needinfo me!)]

This commit broke Linux/clang builds:

make[5]: Entering directory `/home/george/dev/mozilla-central/obj-linux-clang-rel/toolkit/library'
../../accessible/src/atk/nsAccessibleWrap.o: In function `getRoleCB':
/home/george/dev/mozilla-central/accessible/src/atk/nsAccessibleWrap.cpp:738: undefined reference to `nsAccessible::Role()'
../../accessible/src/atk/nsMaiInterfaceAction.o: In function `getKeyBindingCB':
/home/george/dev/mozilla-central/accessible/src/atk/nsMaiInterfaceAction.cpp:111: undefined reference to `nsAccessible::Role()'
/home/george/dev/mozilla-central/accessible/src/atk/nsMaiInterfaceAction.cpp:126: undefined reference to `nsAccessible::Role()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)

Comment 9

[Hubert Figuiere [:hub]]

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=746004

to request a buildbot with clang.

Comment 10

[alexander :surkov (:asurkov)]

Attachment: clang followup

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 615556 [details] [diff] [review]
clang followup

rs=me

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Olli Pettay [:smaug] from comment #11)
> Comment on attachment 615556 [details] [diff] [review]
> clang followup
> 
> rs=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/975341acf9f1

Comment 13

[Mihai Sucan [:msucan]]

Thanks for landing the clang fix! The nsAccessible::Role() error is now gone. However I still get:

/usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv' isn't defined
/usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output

Comment 14

[alexander :surkov (:asurkov)]

Jeff, do you have any ideas?

Comment 15

[Trevor Saunders (:tbsaunde)]

I don't have time to go spec diving atm, but my first thoughts are the following

linker errors in the case that you don't include Accessible-inl.h some where sort of makes sense if the compiler has the right to not emit a function for methods declared inline since then the uses in files that don't incude Accessible-inl.h will have calls to nsAccessible::Role() which was never emitted as a function and always inlined.  Which I think would lead to things like comment 13

On the other hand I can't say that I get these compile errors at all, Alex's patch would probably be my first guess, but I have no idea what the problem was off hand.

Comment 16

[Trevor Saunders (:tbsaunde)]

(In reply to Mihai Sucan [:msucan] from comment #13)
> Thanks for landing the clang fix! The nsAccessible::Role() error is now
> gone. However I still get:
> 
> /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv'
> isn't defined
> /usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output

I assume you've disabled debug symbols, since that looks like a mangled method name?  I wonder if either using gold instead of ld.bfd or building with debug symbols provides more info?

Comment 17

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/975341acf9f1

Comment 18

[Mihai Sucan [:msucan]]

(In reply to Trevor Saunders (:tbsaunde) from comment #16)
> (In reply to Mihai Sucan [:msucan] from comment #13)
> > Thanks for landing the clang fix! The nsAccessible::Role() error is now
> > gone. However I still get:
> > 
> > /usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZN12nsAccessible4RoleEv'
> > isn't defined
> > /usr/bin/ld.bfd.real: final link failed: Nonrepresentable section on output
> 
> I assume you've disabled debug symbols, since that looks like a mangled
> method name?  I wonder if either using gold instead of ld.bfd or building
> with debug symbols provides more info?

For optimized builds I disable debug symbols to make builds faster.

I now re-enabled debug symbols and the errors stays the same.

I did a debug build as well: same error.

I installed gold linker and the error is gone with the patch surkov submitted. If I qpop his patch gold linker gives more info:

make[5]: entrant dans le répertoire « /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library »
/usr/bin/ld.gold.real: error: /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/src/atk/nsAccessibleWrap.o: requires dynamic R_X86_64_PC32 reloc against '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with -fPIC
/usr/bin/ld.gold.real: error: /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/src/atk/nsMaiInterfaceAction.o: requires dynamic R_X86_64_PC32 reloc against '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with -fPIC
/home/mihai/src/mozilla/fx-team/accessible/src/atk/nsAccessibleWrap.cpp:738: error: undefined reference to 'nsAccessible::Role()'
/home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:111: error: undefined reference to 'nsAccessible::Role()'
/home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:126: error: undefined reference to 'nsAccessible::Role()'
clang: error: linker command failed with exit code 1 (use -v to see invocation)
make[5]: *** [libxul.so] Erreur 1


This is on ubuntu 12.04 lts, up-to-date, with debug enabled in mozconfig, with clang 3.0 from the ubuntu repos. I also did an optimized build. Things work well now.


Please let me know how can I further help. Thanks!

Comment 19

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Hub Figuiere [:hub] from comment #9)
> Filed https://bugzilla.mozilla.org/show_bug.cgi?id=746004
> 
> to request a buildbot with clang.

Good.

Note if we haven't fixed the clang issues we should back out the cause until we can find the fix IMO. I use clang on my Linux box and am testing now...

Comment 20

[David Bolter [:davidb] (NeedInfo me for attention)]

I hit the same problem as comment 13. (Ubuntu)

Comment 21

[David Bolter [:davidb] (NeedInfo me for attention)]

Rafael any ideas? (See Linux/clang pain above)

Comment 22

[Trevor Saunders (:tbsaunde)]

> I now re-enabled debug symbols and the errors stays the same.

debug symbols don't let ld.bfd give you unmangled names? :(

> I installed gold linker and the error is gone with the patch surkov
> submitted. If I qpop his patch gold linker gives more info:
> 
> make[5]: entrant dans le répertoire «
> /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library »
> /usr/bin/ld.gold.real: error:
> /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/
> src/atk/nsAccessibleWrap.o: requires dynamic R_X86_64_PC32 reloc against
> '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with
> -fPIC
> /usr/bin/ld.gold.real: error:
> /home/mihai/src/mozilla/fx-team/ff-dbg-obj/toolkit/library/../../accessible/
> src/atk/nsMaiInterfaceAction.o: requires dynamic R_X86_64_PC32 reloc against
> '_ZN12nsAccessible4RoleEv' which may overflow at runtime; recompile with
> -fPIC
> /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsAccessibleWrap.cpp:738:
> error: undefined reference to 'nsAccessible::Role()'
> /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:
> 111: error: undefined reference to 'nsAccessible::Role()'
> /home/mihai/src/mozilla/fx-team/accessible/src/atk/nsMaiInterfaceAction.cpp:
> 126: error: undefined reference to 'nsAccessible::Role()'
> clang: error: linker command failed with exit code 1 (use -v to see
> invocation)
> make[5]: *** [libxul.so] Erreur 1

ok, and his patch should have addressed all of those I think.

> This is on ubuntu 12.04 lts, up-to-date, with debug enabled in mozconfig,
> with clang 3.0 from the ubuntu repos. I also did an optimized build. Things
> work well now.
> 
> 
> Please let me know how can I further help. Thanks!

I'd start by including Accessible-inl.h into random files that don't already include it hoping I'd find one that made things link (Since I can't think of a possible explanation or better idea)

> Note if we haven't fixed the clang issues we should back out the cause until
> we can find the fix IMO. I use clang on my Linux box and am testing now...

I'm not sure I agree, should we also back out anything that breaks solaris, freebsd, or win32 with mingw?

Comment 23

[David Bolter [:davidb] (NeedInfo me for attention)]

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

> > Note if we haven't fixed the clang issues we should back out the cause until
> > we can find the fix IMO. I use clang on my Linux box and am testing now...
> 
> I'm not sure I agree, should we also back out anything that breaks solaris,
> freebsd, or win32 with mingw?

If our developers a finding those significantly useful yes, but I don't think that's the case.

Comment 24

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I have started a build at 7467925b9648f76b7b7dc796ee041c8e6cedff1e using clang 155207. The mozconfig is browser/config/mozconfigs/linux64/debug with

ac_add_options --enable-accessibility
added

Comment 25

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

The build is still going, but from what the error is, it looks like clang is inlining nsAccessible::Role and dropping the symbol. So if some file is using it without including Accessible-inc.h, it will get an undefined reference.

Comment 26

[David Bolter [:davidb] (NeedInfo me for attention)]

Oh I just noticed I'm barfing with:
/usr/bin/ld.bfd.real: libxul.so: hidden symbol `_ZNK14nsWrapperCache10GetWrapperEv' isn't defined
/usr/bin/ld.bfd.real: final link failed: Bad value

Note:

davidb@ubuntu:~/moz/mozilla-inbound$ cd objdir/
davidb@ubuntu:~/moz/mozilla-inbound/objdir$ find . -name *.o |xargs nm |grep _ZNK14nsWrapperCache10GetWrapperEv
         U _ZNK14nsWrapperCache10GetWrapperEv
davidb@ubuntu:~/moz/mozilla-inbound/objdir$

Comment 27

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: proposed patch

It looks like the missing reference is coming from clang not optimizing away the call in

return EnsureInnerWindow() ? GetWrapper() : nsnull;

I have no idea how gold created the library with the missing symbol. In any case,  just including nsWrapperCacheInlines.h in nsGlobalWindow.h fixes the problem for me.

Comment 28

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 617064 [details] [diff] [review]
proposed patch

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

r=me. Thanks!

Comment 29

[Trevor Saunders (:tbsaunde)]

(In reply to David Bolter [:davidb] from comment #28)
> Comment on attachment 617064 [details] [diff] [review]
> proposed patch
> 
> Review of attachment 617064 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. Thanks!

it seems like this fixes your issue in comment 26, but I have a hard time seeing how this bug introduced that, and it seems like it shouldn't fix comment 13?

Comment 30

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

I was only ever able to reproduce the failure in comment 26, are you seeing others?

Comment 31

[Mihai Sucan [:msucan]]

(In reply to Trevor Saunders (:tbsaunde) from comment #22)
> > I now re-enabled debug symbols and the errors stays the same.
> 
> debug symbols don't let ld.bfd give you unmangled names? :(

It seems that no, it doesn't.

(In reply to David Bolter [:davidb] from comment #26)
> Oh I just noticed I'm barfing with:
> /usr/bin/ld.bfd.real: libxul.so: hidden symbol
> `_ZNK14nsWrapperCache10GetWrapperEv' isn't defined
> /usr/bin/ld.bfd.real: final link failed: Bad value

I have also seen this error in some of my tests. Can't remember which.

Comment 32

[(no longer active)]

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