474068 - test_backspace_delete.xul mochitest fails with buggy pango versions

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm trying to go through mochitests and figure out the local failures on my machine.

The one responsible for the bulk of the failures is test_backspace_delete.html.  (This was before the landing earlier today of bug 462188; it's unrelated to the failures there.)

I can reproduce the problem by putting the same characters in an editor.  In particular, the steps to reproduce are:

 1. put the two characters แม (or, as the test has it, the three characters แม่) in a text field (like the one on this bug
 2. press the left and right arrow keys near them

Expected results (which I see on my Mac, and which are what the test expects):  Moving left and right moves the cursor between the point before the แ, the point between the แ and the second character (or the second character cluster, if you used the three character version), and the point after the second character (or second character cluster).

Actual results: the cursor cannot be placed between the characters.


Interestingly, I see the same bug, and the same mochitest failures, in both i686 and x86_64 Linux nightlies on my machine.  I'm not sure why my machine differs from tinderbox, where these tests pass.

Comment 1

[Theppitak Karoonboonyanan]

I cannot reproduce this bug so far on Linux (Debian unstable, amd64), with direct build from trunk.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, I debugged this a little.

IsAcceptableCaretPosition in nsTextFrameThebes.cpp is returning false because gfxTextRun::IsClusterStart is returning false.

So I set a breakpoint in gfxTextRun::CompressedGlyph::SetComplex, which is set because SetGlyphsForCharacterGroup calls aTextRun->GetClusterStart.  Then I got confused because I thought GetClusterStart got its info from the data that was about to be set.  So I decided to try valgrind.

Valgrind told me I was wrong, but that there was a problem elsewhere, which I'll attach.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: valgrind warning

valgrind gives me this warning (in libthai0) the first time I enter Thai text in a text input.  However, it doesn't give the warning again when I enter the problematic text, so I don't think it's actually the problem.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #2)
> So I set a breakpoint in gfxTextRun::CompressedGlyph::SetComplex, which is set
> because SetGlyphsForCharacterGroup calls aTextRun->GetClusterStart.  Then I got
> confused because I thought GetClusterStart got its info from the data that was
> about to be set.  So I decided to try valgrind.

Ah, I see it's not circular because it's called twice, and that's the point where it's updating existing data (with widths, I think).  It's the first call I should be debugging.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #3)
> However, it doesn't give the warning again when I enter the
> problematic text, so I don't think it's actually the problem.

Hmm.  I forgot that valgrind often suppresses repetitions of the same warning (but often doesn't; perhaps it depends on whether the stacks vary slightly).

It turns out changing the text probably does cause the warning multiple times, since I see the difference between:

==22948== ERROR SUMMARY: 9 errors from 4 contexts (suppressed: 8 from 1)
or
==22730== ERROR SUMMARY: 22 errors from 4 contexts (suppressed: 8 from 1)


So this valgrind warning may well be the cause.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #5)
> So this valgrind warning may well be the cause.

I don't think it is.  I patched libthai0 locally, both possible ways (by adding "cur_pos > 0 &&" and "cur_pos == 0 ||", which got rid of the valgrind warning, but not the bug.

Comment 7

[Ginn Chen]

The first character of "แม" is U+0E41, UTF-8 is 0xE0 0xB9 0x81.

In pango/break.c, it has
674		        /* Thai and Lao stuff hardcoded in UAX#29 */
675			if ((wc >= 0x0E40 && wc <= 0x0E44) || (wc >= 0x0EC0 && wc <= 0x0EC4))
676			  GB_type = GB_Prepend; /* Prepend */

Then
719		else if (prev_GB_type == GB_Prepend)
720		  is_grapheme_boundary = FALSE; /* Rule GB9b */

726		attrs[i].is_cursor_position = is_grapheme_boundary;

It is described at
http://unicode.org/reports/tr29/#GB9b

So I think is not a bug. It is a new feature in pango 1.22. It is not yet supported by Windows or Mac OS X.

Comment 8

[Theppitak Karoonboonyanan]

I'm not sure if Thai users will treat this as a "feature". Yes, line should not be broken after such "Prepend" character, but caret movement is a different thing.

In general, users expect the caret to be placable in between this kind of grapheme, and Delete key not to delete the whole grapheme.

See this comment against a Chromium bug for example:
  http://code.google.com/p/chromium/issues/detail?id=3523#c27

I've also seen this problem recently in Gecko after Pango is upgraded in Debian. I'll check Pango soon.

Comment 9

[Ginn Chen]

Thanks for your information.

I saw a bug was logged for ICU.
http://bugs.icu-project.org/trac/ticket/6774

Theppitak, can you file a bug to pango?

Comment 10

[Theppitak Karoonboonyanan]

Filed: http://bugzilla.gnome.org/show_bug.cgi?id=576156

Comment 12

[Zack Weinberg (:zwol)]

So if this is a Pango bug, can we detect the buggy library versions in the test suite and XFAIL the offending eight tests?

Comment 14

[(no longer active)]

We're trying to investigate whether this is the same issue to bug 546636.  From reading this bug, it doesn't have anything to do with whether the HTML5 parser is enabled or not.  Is that right?

Comment 15

[Phil Ringnalda (:philor)]

Hunker down and prepare for heavy bugspam: apparently the new Talos slaves that are going to take over running tests instead of the build slaves have a buggy version of Pango.

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270775591.1270776396.23003.gz
Rev3 Fedora 12 mozilla-central opt test mochitests-4/5 on 2010/04/08 18:13:11
s: talos-r3-fed-032

1264 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Right movement broken in "สสพ่แม่พี่น้อง", offset 5 - got 7, expected 5
1266 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Delete broken in "สสพ่แม่น้อง", offset 5 - got 7, expected 5
1267 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Delete broken in "สสพ่แม่น้อง", offset 5 - got "สสพ่แม่น้อง", expected "สสพ่แพี่น้อง"
1269 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Right movement broken in "สสพ่แม่น้อง", offset 8 - got 9, expected 8
1271 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Delete broken in "สสพ่แม่น้ง", offset 8 - got 9, expected 8
1272 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Delete broken in "สสพ่แม่น้ง", offset 8 - got "สสพ่แม่น้ง", expected "สสพ่แพี่อง"
1274 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | Right movement broken in "สสพ่แม่น้ง", offset 9 - got 10, expected 9
1275 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/generic/test/test_backspace_delete.xul | [SimpleTest/SimpleTest.js, window.onerror] An error occurred - uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIController.doCommand]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://mochi.test:8888/tests/layout/generic/test/test_backspace_delete.xul :: doCommand :: line 60"  data: no] at :0

Comment 16

[Theppitak Karoonboonyanan]

Please try applying the Pango patch proposed in GNOME #576156 (as mentioned in comment #10).

For Debian users, you can try the pre-built debs from here:
  http://ftp.debianclub.org/debclub/pool/main/p/pango1.0/

It's actually Pango's bug, IMO. Let's try to fix GNOME #576156, or even Unicode UAX #29 itself.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270778821.1270779263.29805.gz
Rev3 Fedora 12x64 mozilla-central opt test mochitests-4/5 on 2010/04/08 19:07:01
s: talos-r3-fed64-006

Comment 18

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1270779467.1270781120.1796.gz
Rev3 Fedora 12x64 mozilla-central debug test mochitests-4/5 on 2010/04/08 19:17:47
s: talos-r3-fed64-009

Comment 19

[Armen [:armenzg]]

Hi all,

After reading the bug it seems that this is a permanent orange and therefore we can't reveal the debug mochitest 4/5 suite to developers.

How can we deal with this so we can show that test suites to developers?

As I have read the pango fix is not across linux distributions, therefore, there is nothing we can deploy to the Fedora slaves.

What are the options?
A) Mark the test as KNOWN-FAIL?
B) Don't run the test for Linux?
C) Don't run the test until pango is deployed?

Not sure if I am giving the right options or I understand everything correctly but I want to see what can we do to reveal the hidden test suite until this bug gets fixed.

Comment 20

[Zack Weinberg (:zwol)]

I tried and failed to find contact information for the Pango developers so I could kick them to fix their bug. :-(

How about we modify the test itself so that it flags the incorrect Pango behavior as TODO rather than UNEXPECTED.  That'll green up the mochitest block, and we can pursue the proper fix upstream at our leisure.

Comment 21

[Armen [:armenzg]]

(In reply to comment #20)
> How about we modify the test itself so that it flags the incorrect Pango
> behavior as TODO rather than UNEXPECTED.  That'll green up the mochitest block,
> and we can pursue the proper fix upstream at our leisure.
>

Zack this works for me. Who can do this change?
Thanks.

Comment 22

[Zack Weinberg (:zwol)]

Attachment: conditional todos for the bad results

Well, here's a patch, but who should review it?  Ehsan, maybe?

Comment 23

[Zack Weinberg (:zwol)]

Comment on attachment 440538 [details] [diff] [review]
conditional todos for the bad results

Ehsan says on IRC that he can review this patch.

Comment 24

[(no longer active)]

Comment on attachment 440538 [details] [diff] [review]
conditional todos for the bad results

As discussed on IRC, this patch makes it possible to unintentionally mask some real failures.  The root problem here is that we try to expect some failures, but we don't check what has caused them.

I discussed this on IRC with Zack.  My suggestion is to check to see if the broken pango libraries are in use.  He made the valid point that the problem also happens with some ICU versions (which we don't use right now), and he's not a fan of adding library specific version checks to the code.

While I totally understand those concerns, I simply don't know a better solution here, and I think we shouldn't take this patch as it is right now, because we'd just be opening ourselves up to failures going unnoticed in the future.

Comment 25

[Zack Weinberg (:zwol)]

Attachment: conditional todos v2 (with check for bad pango)

OK, here is a version which uses ctypes to get at pango_version().  If any piece of the check fails we assume we don't have the bug, and even when we do detect the bug, we only switch to TODOs if we observe the _same_ buggy behavior.

When pango_todos is false, the only behavior change should be that doCommand() now traps exceptions and converts them to failures.  There are actually 31 subtest failures due to the Pango bug, but all but six were being masked by an uncaught exception (also caused by the bug - the cursor is not where we expect it to be and so we try to move it past the editable area).

Comment 26

[(no longer active)]

Comment on attachment 440579 [details] [diff] [review]
conditional todos v2 (with check for bad pango)

The patch looks great now, thanks for going through the hassle!

>+	  if (major == 1 && minor >= 22) {

This check worries me, what if it's fixed in a dot-release?  Why can't we just check |major >=1 && minor >= 22| and later on (when the bug is fixed in pango) fix the check to actually cover the broken range?

r=me with that addressed.

Comment 27

[Zack Weinberg (:zwol)]

(In reply to comment #26)
> (From update of attachment 440579 [details] [diff] [review])
> The patch looks great now, thanks for going through the hassle!
> 
> >+	  if (major == 1 && minor >= 22) {
> 
> This check worries me, what if it's fixed in a dot-release?  Why can't we just
> check |major >=1 && minor >= 22| and later on (when the bug is fixed in pango)
> fix the check to actually cover the broken range?

I'm not sure what you're asking for here.  I do hope it will be fixed in a dot-release, and yeah, we're going to have to fix the check when that happens.  Is it just the "assume for now that it will be fixed in 2.0" part that you don't like?  I'd be happy to take that out, but fyi, your suggested rewrited doesn't do that.  It would have to be |major > 1 || (major == 1 && minor >= 22)|.

... Or I could just do the check on the concatenated version number we get back from pango_version, which would get rid of all the decomposition code as well.  That'd just be |version >= 12200|, or |version >= 12200 && version < 20000| for equivalent to what's there now.

> r=me with that addressed.

I'm gonna wait for roc to weigh in before I check anything in, but assuming he likes it too, I think this should go on the 1.9.1 and 1.9.2 branches as well as trunk.  Is that ok with everyone?

Comment 28

[Zack Weinberg (:zwol)]

Attachment: conditional todos v3 (relaxed buggy-library version check, refactored a bit)

Here's a revision on the assumption that what you were asking for, Ehsan, is for the hypothetical Pango 2.0 to also be considered buggy until proven otherwise.  I also factored out the rather messy logic for deciding whether to call |todo_is| to its own function, and added a long comment explaining it.

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 440579 [details] [diff] [review]
conditional todos v2 (with check for bad pango)

>+	// There isn't a ctypes exact equivalent of 'int' :-( so we assume int32.

dwitte says this is no longer true in 3.7.  Though it doesn't matter that much, but probably better to do the right thing on mozilla-central.

Docs for 3.7 behavior at:
https://wiki.mozilla.org/Jsctypes/api#Built-in_types

Comment 30

[Zack Weinberg (:zwol)]

Ok, I'll make that change for the trunk checkin.

Comment 31

[Zack Weinberg (:zwol)]

It occurs to me that this can't go on the 1.9.1 branch because there is no js-ctypes there.  Do we need a workaround for the orange there?

Comment 32

[(no longer active)]

Comment on attachment 440623 [details] [diff] [review]
conditional todos v3 (relaxed buggy-library version check, refactored a bit)

(In reply to comment #28)
> Created an attachment (id=440623) [details]
> conditional todos v3 (relaxed buggy-library version check, refactored a bit)
> 
> Here's a revision on the assumption that what you were asking for, Ehsan, is
> for the hypothetical Pango 2.0 to also be considered buggy until proven
> otherwise.

Yes, that's what I meant.  Thanks!

Comment 33

[(no longer active)]

(In reply to comment #31)
> It occurs to me that this can't go on the 1.9.1 branch because there is no
> js-ctypes there.  Do we need a workaround for the orange there?

Disable the test on Fedora?

I know that it's stupid, but I don't think there is anything better to do on 1.9.1.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I suspect we're not changing the unit test setup on 1.9.1 anyway.

Comment 35

[Zack Weinberg (:zwol)]

If 1.9.1 is sticking with the old unit test boxes, my inclination is to leave it alone.  Or I could just replace all the ctypes goo in the patch with "if (/Linux/.test(navigator.platform)) pango_todos = true;" and then back that out again when the 1.9.1 boxes get a fixed Pango.

cc:ing Mike Hommey, who I know is running unit tests against 3.5 using something other than our infrastructure, for thoughts on that one.

Comment 36

[Zack Weinberg (:zwol)]

http://hg.mozilla.org/mozilla-central/rev/4ce441af2c32
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/b941f5a6931c

... and since we can't do anything in our code about this, I'm inclined to close the bug.  Any objections?

Comment 37

[(no longer active)]

I think that's a fair assessment.  Thanks for your patches!

Comment 38

[Behdad Esfahbod]

(In reply to comment #20)
> I tried and failed to find contact information for the Pango developers so I
> could kick them to fix their bug. :-(

Zack, you just needed to ask around :).  I'm CC'ed here (have not been reading the thread until now), and typically hang around #gfx as behdad.

Anyway, what Pango currently does is following the Unicode specification.  Seems like this is a bug in the Unicode spec.  I'm following up.  Next time, GNOME Bugzilla is your friend.

Comment 39

[Zack Weinberg (:zwol)]

be(In reply to comment #38)
> (In reply to comment #20)
> > I tried and failed to find contact information for the Pango developers so I
> > could kick them to fix their bug. :-(
> 
> Zack, you just needed to ask around :).  I'm CC'ed here (have not been reading
> the thread until now), and typically hang around #gfx as behdad.

Because you were cc:ed here, and on the GNOME bug, I assumed that nagging you in particular would not help.

> Anyway, what Pango currently does is following the Unicode specification. 
> Seems like this is a bug in the Unicode spec.  I'm following up.  Next time,
> GNOME Bugzilla is your friend.

This bug has been reported in GNOME Bugzilla for over a year now, and IIRC you were cc:ed on it from the get-go; I don't think that's fair to Theppitak.

Comment 40

[Behdad Esfahbod]

(In reply to comment #39)
> > Anyway, what Pango currently does is following the Unicode specification. 
> > Seems like this is a bug in the Unicode spec.  I'm following up.  Next time,
> > GNOME Bugzilla is your friend.
> 
> This bug has been reported in GNOME Bugzilla for over a year now, and IIRC you
> were cc:ed on it from the get-go; I don't think that's fair to Theppitak.

Ah, I missed the context.  I didn't remember this bug from last year and assumed that this all happened in recent weeks.

Anyway, I'll work with Thep on the Pango bug to devise a plan to fix the spec and Pango.  In general, requesting a "fix" in pango to diverge from "upstream" spec doesn't get immediate attention on my overloaded TODO list :(.

Cheers,

Comment 41

[Mike Hommey [:glandium]]

(In reply to comment #35)
> If 1.9.1 is sticking with the old unit test boxes, my inclination is to leave
> it alone.  Or I could just replace all the ctypes goo in the patch with "if
> (/Linux/.test(navigator.platform)) pango_todos = true;" and then back that out
> again when the 1.9.1 boxes get a fixed Pango.
> 
> cc:ing Mike Hommey, who I know is running unit tests against 3.5 using
> something other than our infrastructure, for thoughts on that one.

I'm not running mochitests (yet) for various reasons.

nyways, I do think there is a class of problems that just should be handled by local overrides. For example, the external handler xpcshell test only works on linux when the test machine has some specific gnome related configuration in place. Without that, the test fails. There are various tests that fail depending on the environment, and maybe a "simple" way to address this would be for test machine admin to setup a configuration saying "if you get an error on that one, well, just don't care, but still report it". As a result on  tinderbox status, that could be orange with an automatic star. I can file a bug for that if that is thought to be worth.

Comment 42

[Armen [:armenzg]]

Thanks for the great turn around.
WRT 1.9.1, we will probably maintain running the unit tests on CentOS (build machines rather than talos machines).

I am looking at Mochitests 4 and it seems that there is now more tests failing.
I have also requested clobbering the builds just in case something is not been packaged well.

The following builders can be seen in:
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&noignore=1

Fed 12 opt M4:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271935584.1271944602.3686.gz&fulltext=1

Fed 12 debug M4:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271931813.1271939233.18036.gz

Fed 12x64 opt M4:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271933711.1271940820.24084.gz

Fed 12x64 debug M4:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1271938641.1271943844.1457.gz

Comment 43

[Zack Weinberg (:zwol)]

(In reply to comment #42)
> Thanks for the great turn around.
> WRT 1.9.1, we will probably maintain running the unit tests on CentOS (build
> machines rather than talos machines).

Ok, in that case I will not do anything on 1.9.1 for now.

> I am looking at Mochitests 4 and it seems that there is now more tests failing.
> I have also requested clobbering the builds just in case something is not been
> packaged well.

It looks like the code I added does not correctly detect the buggy Pango on these machines; also there is a logic error causing an UNEXPECTED-PASS for the exception.

I need to know the output of these commands on an affected talos machine:

$ grep PANGO_VERSION_ /usr/include/pango-1.0/pango/pango-features.h
$ ls -l /usr/lib/libpango*
$ rpm -qi *pango*

Comment 44

[Armen [:armenzg]]

Not sure if you are going to like this output:
[cltbld@talos-r3-fed-002 ~]$ grep PANGO_VERSION_ /usr/include/pango-1.0/pango/pango-features.h
grep: /usr/include/pango-1.0/pango/pango-features.h: No such file or directory
[cltbld@talos-r3-fed-002 ~]$ ls -l /usr/lib/libpango*
lrwxrwxrwx 1 root root     24 2010-01-20 11:03 /usr/lib/libpango-1.0.so.0 -> libpango-1.0.so.0.2600.0
-rwxr-xr-x 1 root root 297360 2009-09-21 14:32 /usr/lib/libpango-1.0.so.0.2600.0
lrwxrwxrwx 1 root root     29 2010-01-20 11:03 /usr/lib/libpangocairo-1.0.so.0 -> libpangocairo-1.0.so.0.2600.0
-rwxr-xr-x 1 root root  45824 2009-09-21 14:32 /usr/lib/libpangocairo-1.0.so.0.2600.0
lrwxrwxrwx 1 root root     27 2010-01-20 11:03 /usr/lib/libpangoft2-1.0.so.0 -> libpangoft2-1.0.so.0.2600.0
-rwxr-xr-x 1 root root 169196 2009-09-21 14:32 /usr/lib/libpangoft2-1.0.so.0.2600.0
lrwxrwxrwx 1 root root     24 2010-01-20 11:03 /usr/lib/libpangomm-1.4.so.1 -> libpangomm-1.4.so.1.0.30
-rwxr-xr-x 1 root root 183548 2009-09-25 01:20 /usr/lib/libpangomm-1.4.so.1.0.30
lrwxrwxrwx 1 root root     25 2010-01-20 11:03 /usr/lib/libpangox-1.0.so.0 -> libpangox-1.0.so.0.2600.0
-rwxr-xr-x 1 root root  47992 2009-09-21 14:32 /usr/lib/libpangox-1.0.so.0.2600.0
lrwxrwxrwx 1 root root     27 2010-01-20 11:03 /usr/lib/libpangoxft-1.0.so.0 -> libpangoxft-1.0.so.0.2600.0
-rwxr-xr-x 1 root root  30072 2009-09-21 14:32 /usr/lib/libpangoxft-1.0.so.0.2600.0
[cltbld@talos-r3-fed-002 ~]$ rpm -qi *pango*
package *pango* is not installed

Comment 45

[Zack Weinberg (:zwol)]

This is what I get for assuming things are in the same place on a Redhat-based system that they are on a Debian-based system.  The first of those commands was in some ways the most important, and if you can find where pango-features.h is hiding and repeat that "grep" with the right pathname I would much appreciate it.

However, the command that did work *may* have given me enough information to fix the problem.  Stay tuned.

Comment 46

[Zack Weinberg (:zwol)]

Attachment: follow-up fix

Here's a patch that should address the problems on the Fedora talos boxes.  The logic for converting exceptions into failures was slightly wrong, and more importantly, I got the official soname for libpango wrong.  (ctypes really needs an equivalent of dlsym(RTLD_DEFAULT, ...).)

This works on my machine, but so did the previous patch :-/  Should I just push this and see what happens?

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Some of the differences are probably related to whether you have -dev packages installed.  From Ubuntu 9.10:

$ dpkg -S /usr/lib/libpango-1.0.so*
libpango1.0-dev: /usr/lib/libpango-1.0.so
libpango1.0-0: /usr/lib/libpango-1.0.so.0
libpango1.0-0: /usr/lib/libpango-1.0.so.0.2600.0

... so it wouldn't have worked on Ubuntu without the -dev package either.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 440800 [details] [diff] [review]
follow-up fix

And yes, you should just push this.  r=dbaron too.

(And I agree that (exc_todo ? todo : ok) is the right pattern here.)

Comment 49

[Zack Weinberg (:zwol)]

(In reply to comment #47)
> Some of the differences are probably related to whether you have -dev packages
> installed.  From Ubuntu 9.10:
> 
> $ dpkg -S /usr/lib/libpango-1.0.so*
> libpango1.0-dev: /usr/lib/libpango-1.0.so
> libpango1.0-0: /usr/lib/libpango-1.0.so.0
> libpango1.0-0: /usr/lib/libpango-1.0.so.0.2600.0
> 
> ... so it wouldn't have worked on Ubuntu without the -dev package either.

I was confused by the nonstandard naming convention (it ought to be libpango.so, libpango.so.1, and libpango.so.1.26.0) and thought "libpango-1.0.so" was the soname link rather than the dev link.  Also it didn't occur to me that the talos machines (unlike the build machines) don't *need* the -dev packages installed.

I'll push to m-c as soon as the sheriff OKs it, and to 1.9.2 after we've confirmed that this fixes the problem.

Comment 50

[[e-mail]]

Comment on attachment 440800 [details] [diff] [review]
follow-up fix

http://hg.mozilla.org/mozilla-central/rev/49619058edca

Comment 51

[Zack Weinberg (:zwol)]

I see green mochitest-4 in the "Fedora 12" columns of http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox&noignore=1 so I've pushed the follow-up fix to 1.9.2: http://hg.mozilla.org/releases/mozilla-1.9.2/rev/488616ffdc6a

Comment 52

[Al Billings [:abillings - ex-MoCo]]

This is passing on the 1.9.2 Linux tinderboxen running mochitests (see http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.6/1272192871.1272194954.13226.gz&fulltext=1). Is that enough to verify this bug or do we need to track down a machine with the bad libraries to run the STR by hand?

Comment 53

[(no longer active)]

I think it's enough for verification if you make sure that the machines with the bad library versions are passing.  You don't need to test things manually.

Comment 54

[Al Billings [:abillings - ex-MoCo]]

Well, the $64,000 question is which machines have the bad library versions. Do all of the x86 Fedora-based tinderbox machines have the bad library or only some of them?

Comment 55

[(no longer active)]

Well, talos-r3-fed-032 was the machine running the test in comment 15, for example.  Does that help?

Comment 56

[Armen [:armenzg]]

All of them, these are the new Rev3 Fedora machines. This wasn't happening on the CentosOS unit tests.

For further clarification, we are currently running unit tests on all branches in the CentosOS machines. Additionally, we are running unit tests on Fedora machines only for mozilla-central. Therefore, we are running unit tests for m-c on CentosOS *and* Fedora 12. At some point, when we have no perma-oranges on Fedora we will switch unit tests off for CentosOS on m-c and gradually completely switching over to running unit tests only on the Fedora machines.

Not sure if you needed to know all the context but I hope it helps in case you are trying to determine what is going on.

Comment 57

[Al Billings [:abillings - ex-MoCo]]

Armen, is the 1.9.2 branch still using the CentOS boxes? I'm concerned with 1.9.2, not m-c.

Comment 58

[Armen [:armenzg]]

Yes, 1.9.2 is covered by CentOS boxes.

Comment 59

[Al Billings [:abillings - ex-MoCo]]

I'll have to find a machine somewhere to verify this by hand then since none of the 1.9.2 machines are going to exhibit this problem.

Comment 60

[Zack Weinberg (:zwol)]

Armen: I had been under the impression that 1.9.2 was eventually going to switch to F12 unit-tests-on-talos, like m-c, or I wouldn't have bothered putting the fix on the branch.  Is that planned to happen, or?

Al: I can walk you through manual verification if you can get your hands on an appropriate machine.  There have been several GNOME releases since the buggy Pango was introduced; any Linux distribution shipped in the past year or so should have the problem.  An easy way to check is to run this command:

$ echo /usr/lib/libpango-1.0.so.0.*
/usr/lib/libpango-1.0.so.0.2800.0

If the four-digit number (2800 in this case) is greater than or equal to 2200, the bug should be present.

Comment 61

[Armen [:armenzg]]

(In reply to comment #60)
> Armen: I had been under the impression that 1.9.2 was eventually going to
> switch to F12 unit-tests-on-talos, like m-c, or I wouldn't have bothered
> putting the fix on the branch.  Is that planned to happen, or?
> 
This is the plan. We are currently blocked on a new order of Rev3 machines (talos/unit tests) before we can enable this on 1.9.2.
I am sorry if I gave the wrong answer or did not understand the question.

Comment 62

[Zack Weinberg (:zwol)]

Yes, that's what I wanted to know. Thanks for clarifying.

Comment 63

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> I'm not sure if Thai users will treat this as a "feature". Yes, line should not
> be broken after such "Prepend" character, but caret movement is a different
> thing.
> 
> In general, users expect the caret to be placable in between this kind of
> grapheme, and Delete key not to delete the whole grapheme.

Theppitak, is there a time when the Unicode recommended algorithm should be used?
http://www.unicode.org/reports/tr29/tr29-17.html#Default_Grapheme_Cluster_Table
For example, should CSS first-letter apply to the whole of แม?

If so, that would mean we need have different kinds of "clusters".

From where does Gecko's backspace deletes code-points but Delete deletes clusters concept come?  Is that because the base character usually is the first code point (with Prepend being an exception), and removing the base character leaves the other code points meaningless (so they are also removed)?

Comment 64

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, that's one reason.

Also, pressing a letter key usually inserts a character (let's ignore IMEs here), and apparently there's a user assumption that one backspace undoes the last letter key press. So backspace should delete characters. There's no similar constraint for the "delete" key.

Comment 65

[Theppitak Karoonboonyanan]

(In reply to comment #63)

> Theppitak, is there a time when the Unicode recommended algorithm should be
> used?
> http://www.unicode.org/reports/tr29/tr29-17.html#Default_Grapheme_Cluster_Table

None. The UAX#29 practice for Prepend and SpacingMark is never used anywhere I know of in real life. The line breaking is the only desirable side-effect I can imagine (whose benefit is quite small). But it's just wrong elsewhere.

> For example, should CSS first-letter apply to the whole of แม?

I can show you a sample from a Thai journal, where the word "เมื่อ" is the first word of the article, and only "เ" is shown as the drop cap.

> If so, that would mean we need have different kinds of "clusters".

No. We need to fix UAX#29 instead. That kind of "cluster" is against all existing usages.

> From where does Gecko's backspace deletes code-points but Delete deletes
> clusters concept come?  Is that because the base character usually is the first
> code point (with Prepend being an exception), and removing the base character
> leaves the other code points meaningless (so they are also removed)?

As Robert has explained in comment 64, that's right. And a Prepend/SpacingMark character is treated as a unit in its own rights, according to the so-called "visual encoding" scheme. Otherwise, the complete cluster of "เมื่อ" must be "เมื่อ|", not "เมื่|อ|" because "เ-ือ" itself is a compound vowel. But Thai does not use that scheme in its national standards (from which the Thai Unicode specification was defined). The so-called "visual encoding" just treats every vertical stack of characters as a single unit, and no cluster takes more than one vertical stack.

Comment 66

[Theppitak Karoonboonyanan]

Attachment: Thai drop cap sample

(In reply to comment #65)
> I can show you a sample from a Thai journal, where the word "เมื่อ" is the
> first word of the article, and only "เ" is shown as the drop cap.

And here it is.

Comment 67

[Theppitak Karoonboonyanan]

Attachment: Thai dropcap sample 2

Another sample, illustrating a case of "ใน".