Bug 38648 - Ensure DRT loads GAIL (Gtk+ module), for a11y tests
: Ensure DRT loads GAIL (Gtk+ module), for a11y tests
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To: Nobody
:
Depends on: 39165 41346 42103
Blocks: 31018
  Show dependency treegraph
 
Reported: 2010-05-06 08:50 PDT by Mario Sanchez Prada
Modified: 2010-07-12 17:25 PDT (History)
5 users (show)

See Also:


Attachments
Patch to fix this issue (1.87 KB, patch)
2010-05-06 10:47 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch to fix this issue (1.87 KB, patch)
2010-05-06 10:55 PDT, Mario Sanchez Prada
xan.lopez: review-
Details | Formatted Diff | Diff
Patch to fix this issue (1.85 KB, patch)
2010-05-07 03:09 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff
Patch to fix this issue (1.93 KB, patch)
2010-06-30 01:38 PDT, Mario Sanchez Prada
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mario Sanchez Prada 2010-05-06 08:50:42 PDT
After some investigation while working for fixing bug 31018, I found that the AtkObject* returned by AccessibilityController::rootElement() -in AccessibilityControllerGtk.cpp- is *not a valid AtkObject* at all (ATK_ROLE_INVALID), which makes impossible to later access to its only child and therefore any AccessibleUIElement associated to the DOM's elements.

As a consecuence of this, it was not possible to implement the isSelected() method in DRT to make the accessibility/aria-controls-with-tabs.html test success, as it was not possible to access to any AccessibleUIElement in the DOM starting from the root element.

After some investigation I found the problem was that the GAIL Gtk module was not being loaded along with the execution of DRT (even if the environment variable was already set), and that's the reason behind those AtkObjects returned by rootElement() not being valid.

Therefore, we need to make sure GAIL is always loaded with DRT if we want this kind of a11y tests (using the AccessibleUIElements starting from the root element) work properly.

As side note: it seems that some a11y apps like Orca and Accerciser manually make sure the GTK_MODULES envvar is properly loaded along with their executions, so perhaps that's the way to go here, providing it would only affect to the GTK implementation of the DRT.
Comment 1 Mario Sanchez Prada 2010-05-06 09:08:06 PDT
Blocking bug 31018 as this issue directly affects to the possibility of implementing the isSelected() method in Gtk's DRT
Comment 2 Mario Sanchez Prada 2010-05-06 10:47:58 PDT
Created attachment 55265 [details]
Patch to fix this issue

This patch makes sure that the Gtk version of the DRT always set the GTK_MODULES variable to the proper value ("gail") before calling to gtk_init(), in a similar fashion to how Orca an Accerciser does it.
Comment 3 WebKit Review Bot 2010-05-06 10:49:06 PDT
Attachment 55265 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/gtk/DumpRenderTree.cpp:912:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Mario Sanchez Prada 2010-05-06 10:55:04 PDT
Created attachment 55267 [details]
Patch to fix this issue

(In reply to comment #3)
> Attachment 55265 [details] did not pass style-queue:
> 

Sorry, forgot to check style. Uploading new patch
Comment 5 Xan Lopez 2010-05-07 01:32:15 PDT
Comment on attachment 55267 [details]
Patch to fix this issue

We agreed to spend some time investigating why GTK_MODULES is being thrown away by the test scripts, and whether or not it makes sense to try to be more careful when setting it in the application.
Comment 6 Mario Sanchez Prada 2010-05-07 03:09:29 PDT
Created attachment 55354 [details]
Patch to fix this issue

According to comment #5 I've been spending some time to find out why DRT is ran without considering the GTK_MODULES envvar (if already set) and I found the reason:

In WebKitTools/Scripts/old-run-webkit-tests > openDumpTool() a clean environment is prepared before calling to DRT, which basically creates a hashtable with the strictly needed envvars and nothing else, and that obviously doesn't include GTK_MODULES at this point.

So, I think the current patch properly addresses the problem by just setting the GTK_MODULES envvar to "gail" when running the tests for the Gtk port, as that would be a clear requirement for the a11y tests to run ok under that scenario.

Wrt the doubts also commented with xan about whether to append 'gail' to the already present (if so) GTK_MODULES instead of just *setting* it to a specific value, I'd say that the right thing to do in this case is just to set it, since we don't want to put in that clean environment anything else but the strictly needed things, which in this case means (at least afaik) to load Gail as the only Gtk module.

Hope this patch looks better :-)
Comment 7 Xan Lopez 2010-05-10 10:01:43 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

Looks good to me, we just need to remember to update the new script when it supports GTK+.
Comment 8 WebKit Commit Bot 2010-05-10 10:17:22 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

Rejecting patch 55354 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--test', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 55354, '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
5354&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=38648&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 55354 from bug 38648.
NOBODY (OOPS!) found in /Users/eseidel/Projects/CommitQueue/WebKitTools/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKitTools/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 9 Adam Barth 2010-05-15 00:03:45 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

Eric said this rejection was caused by a bug in svn-apply, which hopefully has been fixed.
Comment 10 WebKit Commit Bot 2010-05-15 11:26:57 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

Clearing flags on attachment: 55354

Committed r59544: <http://trac.webkit.org/changeset/59544>
Comment 11 WebKit Commit Bot 2010-05-15 11:27:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2010-05-15 11:45:51 PDT
http://trac.webkit.org/changeset/59544 might have broken GTK Linux 32-bit Release
Comment 13 Adam Barth 2010-05-15 11:49:26 PDT
editing/undo/4059423-1.html crashed.  Same error on GTK Linux 64-bit Release.  Rolling out.
Comment 14 Eric Seidel 2010-05-16 13:36:41 PDT
re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.
Comment 15 Mario Sanchez Prada 2010-05-25 09:34:27 PDT
(In reply to comment #14)
> re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.

I can't reproduce this in my 32-bit compilation of WebKit (Gtk version), with that patch applied:

$ run-webkit-tests --gtk editing/undo/4059423-1.html
Running build-dumprendertree
Running tests from /home/mario/work/gnome2/WebKit/LayoutTests
Testing 1 test cases.
editing/undo .
0.38s total testing time

all 1 test cases succeeded


Am I missing something, perhaps?
Comment 16 Mario Sanchez Prada 2010-06-10 08:09:55 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > re-opening since rolled out.  We need to fix the commit-queue to re-open the original bug when landing rollout patches.
> 
> I can't reproduce this in my 32-bit compilation of WebKit (Gtk version), with that patch applied:

Strike two:

I have checked this several times again, and didn't find anything wrong with that. Moreover, considering the nature of this patch (which just adds a envvar to the script for running the tests) I'm a bit confused about how this could be causing a problem at all. It looks to me more like an issue of flackyness or something like that than this patch breaking the build.

Could someone help me to clarify this? Perhaps I'm not seeing anything or not doing the proper tests, and that's all. But it would be nice to have this issue fixed as it's blocking some a11y tests in the GTK port since some stuff from DRT won't work properly without it.

Thanks
Comment 17 Xan Lopez 2010-06-29 04:51:27 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

I can't reproduce the crash locally either, so let's try to get this in again.
Comment 18 Mario Sanchez Prada 2010-06-29 05:07:12 PDT
(In reply to comment #17)
> (From update of attachment 55354 [details])
> I can't reproduce the crash locally either, so let's try to get this in again.

Cross fingers! :-)
Comment 19 WebKit Commit Bot 2010-06-29 05:39:23 PDT
Comment on attachment 55354 [details]
Patch to fix this issue

Clearing flags on attachment: 55354

Committed r62106: <http://trac.webkit.org/changeset/62106>
Comment 20 WebKit Commit Bot 2010-06-29 05:39:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 WebKit Review Bot 2010-06-29 06:07:00 PDT
http://trac.webkit.org/changeset/62106 might have broken GTK Linux 32-bit Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/62105
http://trac.webkit.org/changeset/62106
Comment 22 Mario Sanchez Prada 2010-06-29 09:17:55 PDT
(In reply to comment #21)
> http://trac.webkit.org/changeset/62106 might have broken GTK Linux 32-bit Release
> The following changes are on the blame list:
> http://trac.webkit.org/changeset/62105
> http://trac.webkit.org/changeset/62106

The problem behind this broken build is not this bug, but the fact that the new tests added along with bug 36128 need to be updated to match the new code added in patch for bug 40009 (now a11y title and descriptions are prefixed by "AXTitle: " and "AXDescription: ", respectively).

Hence, now reopening bug 40009 and attaching the needed patch to updated the failing test and its expected results: platform/gtk/accesibility/name-for-label.
Comment 23 Mario Sanchez Prada 2010-06-29 09:25:29 PDT
(In reply to comment #22)
> [...]
> Hence, now reopening bug 40009 and attaching the needed patch to updated the failing test and its expected results: platform/gtk/accesibility/name-for-label.

Much better, filed a new bug to fix this small issue:
https://bugs.webkit.org/show_bug.cgi?id=41355
Comment 24 Mario Sanchez Prada 2010-06-30 01:38:05 PDT
Created attachment 60098 [details]
Patch to fix this issue

It looks like this patch has been rolled out because it seems it broke some tests:

https://bugs.webkit.org/show_bug.cgi?id=41346
http://trac.webkit.org/changeset/62110

I had no idea what could went wrong there apart that there was a missing ";" in the previous patch (I didn't realize of this since it worked in my local environment), so I'm now attaching a new patch with that simple modification to see if it helps, but no many hopes put down on there...

Btw, any tip on this would be welcome, since this bug is blocking other a11y fixes and if I can't reproduce the problem locally I'm not sure I'll be able to find the proper solution for that.

Thanks
Comment 25 Mario Sanchez Prada 2010-06-30 23:25:53 PDT
(In reply to comment #24)
> Created an attachment (id=60098) [details]
> Patch to fix this issue
> 
> It looks like this patch has been rolled out because it seems it broke some tests:

Btw, shouldn't we reopen this bug then?
Comment 26 Xan Lopez 2010-07-12 02:25:33 PDT
Comment on attachment 60098 [details]
Patch to fix this issue

For the horde.
Comment 27 Xan Lopez 2010-07-12 10:42:09 PDT
Reopen so the bot pushes the patch.
Comment 28 WebKit Commit Bot 2010-07-12 11:18:46 PDT
Comment on attachment 60098 [details]
Patch to fix this issue

Clearing flags on attachment: 60098

Committed r63101: <http://trac.webkit.org/changeset/63101>
Comment 29 WebKit Commit Bot 2010-07-12 11:18:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 30 Eric Seidel 2010-07-12 13:58:58 PDT
I think this caused Gtk API test failures.
Comment 31 Mario Sanchez Prada 2010-07-12 14:08:36 PDT
(In reply to comment #30)
> I think this caused Gtk API test failures.

I'm currently trying to reproduce the error in my local machine but no success so far. Moreover, I've to recognize I'm a bit lost with this issue (which seems to be doomed :-)) since I can't see why a change in old-run-webkit-tests script can affect to the API tests which are supposed not to use this script, afaik.

Now rebasing the patch against last commit from trunk in my local machine, to see if I can reproduce it... sorry for all the trouble coming all the time with this patch :-(
Comment 32 Xan Lopez 2010-07-12 14:22:33 PDT
This was rolled out and the API test still fails, reopening.
Comment 33 Xan Lopez 2010-07-12 14:23:00 PDT
Comment on attachment 60098 [details]
Patch to fix this issue

Hopefully last time we do this :D
Comment 34 Mario Sanchez Prada 2010-07-12 14:42:06 PDT
(In reply to comment #32)
> This was rolled out and the API test still fails, reopening.

I can reproduce the API fail with last trunk, regardless of this patch being applied or not, so the problem is somewhere else for sure...
Comment 35 WebKit Commit Bot 2010-07-12 15:51:53 PDT
Comment on attachment 60098 [details]
Patch to fix this issue

Clearing flags on attachment: 60098

Committed r63133: <http://trac.webkit.org/changeset/63133>
Comment 36 WebKit Commit Bot 2010-07-12 15:52:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 37 Mario Sanchez Prada 2010-07-12 17:01:04 PDT
(In reply to comment #36)
> All reviewed patches have been landed.  Closing bug.

JFTR, I've been bisecting for a while and found that the guilty commit breaking the API test was this one:

http://trac.webkit.org/changeset/63100

I don't know whether that commit actually broke something in API tests or just unveiled a problem somewhere else, but git bisect doesn't lie and before that commit everything worked fine wrt those tests.

My 2 cents so far
Comment 38 Mario Sanchez Prada 2010-07-12 17:25:17 PDT
(In reply to comment #37)
> (In reply to comment #36)
> > All reviewed patches have been landed.  Closing bug.
> 
> JFTR, I've been bisecting for a while and found that the guilty commit breaking the API test was this one:
> 
> http://trac.webkit.org/changeset/63100
> 
> I don't know whether that commit actually broke something in API tests or just unveiled a problem somewhere else, but git bisect doesn't lie and before that commit everything worked fine wrt those tests.
> 
> My 2 cents so far

Hmmm.. looks like I was too slow, as Martin already filed a bug for this: 

https://bugs.webkit.org/show_bug.cgi?id=42114

Sorry for the spam