don't export nsARIAMap.h

RESOLVED FIXED in mozilla14

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 1 bug)

unspecified
mozilla14
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 613293 [details] [diff] [review]
patch
Attachment #613293 - Flags: review?(trev.saunders)
(Assignee)

Comment 1

5 years ago
Created attachment 613868 [details] [diff] [review]
patch2

fix build issues
Attachment #613293 - Attachment is obsolete: true
Attachment #613293 - Flags: review?(trev.saunders)
Attachment #613868 - Flags: review?(trev.saunders)
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"
>
Attachment #613868 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 3

5 years ago
(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.
(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"?
(Assignee)

Comment 5

5 years ago
(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.
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0975fb1e62d
https://hg.mozilla.org/mozilla-central/rev/c0975fb1e62d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
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)
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=746004

to request a buildbot with clang.
(Assignee)

Comment 10

5 years ago
Created attachment 615556 [details] [diff] [review]
clang followup
Attachment #615556 - Flags: review?(trev.saunders)
Comment on attachment 615556 [details] [diff] [review]
clang followup

rs=me
Attachment #615556 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 12

5 years ago
(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
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
(Assignee)

Comment 14

5 years ago
Jeff, do you have any ideas?
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.
(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?
https://hg.mozilla.org/mozilla-central/rev/975341acf9f1
(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!
(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...
I hit the same problem as comment 13. (Ubuntu)
Rafael any ideas? (See Linux/clang pain above)
> 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?
(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.
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
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.
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$
Created attachment 617064 [details] [diff] [review]
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.
Attachment #617064 - Flags: review?(dbolter)
Comment on attachment 617064 [details] [diff] [review]
proposed patch

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

r=me. Thanks!
Attachment #617064 - Flags: review?(dbolter) → review+
(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?
I was only ever able to reproduce the failure in comment 26, are you seeing others?
(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.
https://hg.mozilla.org/mozilla-central/rev/ab142890b086
You need to log in before you can comment on or make changes to this bug.