Failure when running gtests locally on a debug build

RESOLVED FIXED in mozilla30

Status

()

Core
Graphics: Layers
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: nical)

Tracking

unspecified
mozilla30
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments)

Created attachment 8372278 [details]
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.
Created attachment 8372279 [details]
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.
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?
Summary: Failure when running gtests locally → Failure when running gtests locally on a debug build
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
Created attachment 8372305 [details]
Backtrace for warning

Nical, any thoughts? I assume you know this code reasonably well - feel free to bump it to somebody else if not.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Comment 5

4 years ago
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.
Flags: needinfo?(nical.bugzilla)
(Assignee)

Updated

4 years ago
Assignee: nobody → nical.bugzilla
(Assignee)

Comment 6

4 years ago
Created attachment 8373246 [details] [diff] [review]
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.
Attachment #8373246 - Flags: review?(bugmail.mozilla)
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 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.
Attachment #8373246 - Flags: review?(bugmail.mozilla) → review+
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.

Updated

4 years ago
Duplicate of this bug: 950383
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)
(Assignee)

Updated

4 years ago
Blocks: 968807
(Assignee)

Comment 12

4 years ago
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Whiteboard: [leave open]
Created attachment 8383284 [details] [diff] [review]
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.)
Attachment #8383284 - Flags: review?(bugmail.mozilla)
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.
Attachment #8383284 - Flags: review?(bugmail.mozilla) → review?(jdaggett)

Comment 17

4 years ago
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.
Attachment #8383284 - Flags: review?(jdaggett) → review-

Comment 18

4 years ago
Created attachment 8383380 [details] [diff] [review]
patch, null check bundle id with warning

Null check earlier and spit out a warning.
Attachment #8383380 - Flags: review?(smichaud)
Nice.  Kats, this wouldn't account for Linux problems that you originally had - did they go away with Nicolas' patch?
Flags: needinfo?(bugmail.mozilla)
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.
Attachment #8383380 - Flags: review?(smichaud) → review+
Attachment #8373246 - Flags: checkin+
Flags: needinfo?(bugmail.mozilla)
(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.
(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.
> 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?
>> 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.
So, should we land the r+'d patch, or wait until we figure out the root cause?
Flags: needinfo?(smichaud)
Flags: needinfo?(jdaggett)
> 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.
Flags: needinfo?(smichaud)

Updated

4 years ago
Flags: needinfo?(jdaggett)
(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!
Keywords: checkin-needed
(Assignee)

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.