969388 - Failure when running gtests locally on a debug build

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Output from gtest

When I run 'mach gtest' locally on OS X and Linux I get a failure. The output from the gtest harness says "TEST-UNEXPECTED-FAIL" at the top but not in the output below; it seems to crap out after starting the Layers.TextureSerialization test. I haven't dug into this yet.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Output under debugger

Of course, when I run under a debugger it runs to completion. Although there are a couple of errors and various warnings/assertions.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

The errors at the end are because of a patch I had applied locally; ignore them. Also I'm running on a debug build, which is presumably why the warnings in TextureSerialization are causing it to crap out?

Comment 3

[Doug Sherk (:drs) (inactive)]

I am able to repro this as well. My mozconfig is:

export MOZ_RUN_GTEST=1
ac_add_options --disable-optimize
ac_add_options --enable-tests
ac_add_options --enable-debug

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Backtrace for warning

Nical, any thoughts? I assume you know this code reasonably well - feel free to bump it to somebody else if not.

Comment 5

[Nicolas Silva [:nical]]

In TestTextures.cpp, GetAsSurface is called before the texture is Locked, which is bad. I think we just need to move up the Lock call that is right below.

Comment 6

[Nicolas Silva [:nical]]

Attachment: Fix locking in the test

This makes sure we lock before GetAsSurface, UpdateSurface and UpdateYCbCr, and fixes the open mode to be read and write rather than read only since we are about to write into the texture.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

This patch removes the warnings I was seeing before, but it doesn't seem to fix the crash. Frankly I don't know why it's still crashing because when I run it under lldb it seems to pass fine.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8373246 [details] [diff] [review]
Fix locking in the test

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

r+ since it's an improvement but I don't think it should land yet because it doesn't fix the full bug.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Also I verified that when I comment out the TextureSerialization test in TestTextures.cpp gtest runs to completion successfully. So there's definitely a problem in that test.

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Incidentally on Linux I also see this output from the test (without the above patch):

[ RUN      ] Layers.TextureSerialization

(process:5089): GLib-GObject-CRITICAL **: /build/buildd/glib2.0-2.32.4/./gobject/gtype.c:2722: You forgot to call g_type_init()

(process:5089): GLib-CRITICAL **: g_once_init_leave: assertion `result != 0' failed

(process:5089): Gdk-CRITICAL **: IA__gdk_x11_display_get_xdisplay: assertion `GDK_IS_DISPLAY (display)' failed
[5089] WARNING: GetAsDrawTarget should be called on locked textures only: 'mLocked', file /home/kats/zspace/mozilla-git-2/gfx/layers/client/TextureClient.cpp, line 529
[5089] WARNING: GetAsDrawTarget should be called on locked textures only: 'mLocked', file /home/kats/zspace/mozilla-git-2/gfx/layers/client/TextureClient.cpp, line 529
[5089] WARNING: GetAsDrawTarget should be called on locked textures only: 'mLocked', file /home/kats/zspace/mozilla-git-2/gfx/layers/client/TextureClient.cpp, line 529
[       OK ] Layers.TextureSerialization (41 ms)

Comment 12

[Nicolas Silva [:nical]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #8)
> r+ since it's an improvement but I don't think it should land yet because it
> doesn't fix the full bug.

Sorry, landed it because it blocks bug 968807 which I want to get landed asap. Marking the bug leave open to continue investigations.

https://hg.mozilla.org/integration/mozilla-inbound/rev/1a5dbdd480ff

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1a5dbdd480ff

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Guard against null mainBundleID

This fixes it for me OS X 10.9 at least - TextureSerialization is the test that triggers the crash because it initializes gfxPlatform to get to the backend preferences. That creates gfxPlatformMac, which gets null mainBundleID from a call to ::CFBundleGetIdentifier, and crashes in the call to CTFontManagerSetAutoActivationSetting (see bug 922590 for a standalone test where this kind of a call passes on 10.8, but fails on 10.9, pointing to an Apple bug.)

Comment 15

[Milan Sreckovic [:milan] (needinfo for best results)]

https://tbpl.mozilla.org/?tree=Try&rev=9405a8f6593a

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8383284 [details] [diff] [review]
Guard against null mainBundleID

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

I know nothing about this code; bouncing to jdaggett who is the most recent toucher of the code according to blame.

Comment 17

[John Daggett (:jtd)]

Comment on attachment 8383284 [details] [diff] [review]
Guard against null mainBundleID

Looks fine but we really should spit out a warning.  As others have noted on bug 922590, not having a bundle id really means that something is not quite copacetic about the packaging.  And we should probably bail immediately after looking up the bundle id.

Comment 18

[John Daggett (:jtd)]

Attachment: patch, null check bundle id with warning

Null check earlier and spit out a warning.

Comment 19

[Milan Sreckovic [:milan] (needinfo for best results)]

Nice.  Kats, this wouldn't account for Linux problems that you originally had - did they go away with Nicolas' patch?

Comment 20

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 8383380 [details] [diff] [review]
patch, null check bundle id with warning

Sounds reasonable to me.

Of course the real question is how the main bundle ID can be null here.

> 10.9 crash in CTFontManagerSetAutoActivationSetting

Anyone have a crash stack for this?  I can't help wondering if this might be related to bug 951906, which I'm currently working on.

Comment 21

[Kartikaya Gupta (email:[e-mail])]

(In reply to Milan Sreckovic [:milan] from comment #19)
> Nice.  Kats, this wouldn't account for Linux problems that you originally
> had - did they go away with Nicolas' patch?

The Linux warnings I see still happen on central (which has Nicolas' patch). However the tests do run to completion on Linux, so it's not as big of a problem there.

Comment 22

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Steven Michaud from comment #20)
> ...
> Of course the real question is how the main bundle ID can be null here.
> 
> > 10.9 crash in CTFontManagerSetAutoActivationSetting
> 
> Anyone have a crash stack for this?  I can't help wondering if this might be
> related to bug 951906, which I'm currently working on.

It happens in gfxPlatformMac::gfxPlatformMac, called by gfxPlatform::Init(), but only when running the unit tests using "mach gtest".  Running the executable directly is fine.

Bug 922590 has another stack, when the bundle ID is explicitly set to null.

Comment 23

[Steven Michaud [:smichaud] (Retired)]

> Bug 922590 has another stack,

Interesting.  The top three lines of that stack (in bug 922590 comment #2) are the same as those for bug 951906 ... but only those top three.

> when the bundle ID is explicitly set to null.

What do you mean by this?

Comment 24

[Steven Michaud [:smichaud] (Retired)]

>> when the bundle ID is explicitly set to null.
>
> What do you mean by this?

Never mind.  I assume you mean this happens when a main bundle's Info.plist doesn't have any CFBundleIdentifier key.

Comment 25

[Milan Sreckovic [:milan] (needinfo for best results)]

So, should we land the r+'d patch, or wait until we figure out the root cause?

Comment 26

[Steven Michaud [:smichaud] (Retired)]

> So, should we land the r+'d patch, or wait until we figure out the root cause?

How about this:

We land the patch now and close this bug, but if the patch causes/uncovers other problems we reopen the bug and try to dig deeper.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Steven Michaud from comment #26)
> > So, should we land the r+'d patch, or wait until we figure out the root cause?
> 
> How about this:
> 
> We land the patch now and close this bug, but if the patch causes/uncovers
> other problems we reopen the bug and try to dig deeper.

Sounds good!

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4ac5c87912d0

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4ac5c87912d0