515595 - Resolve how to initialise layout correctly in xpcshell tests from within gfx especially for non-libxul builds

Status: RESOLVED INVALID

(Core :: Graphics, defect)

Description

[Mark Banner (:standard8)]

After bug 504034 Thunderbird is seeing test failures in test_nsIScriptableRegion.js due to xpcshell crashing.

I've so far had a quick look in the debugger and it is due to this:

 void* nsRegion::RgnRect::operator new (size_t) CPP_THROW_NEW
 {
-  return gRectPool.Alloc ();
+  return gRectPool->Alloc ();
 }

gRectPool is null. Firefox unit tests don't seem affected by it, but all platforms of Thunderbird do.

This could be a libxul versus non-libxul issue, though I'm not quite sure yet.

Comment 1

[Mark Banner (:standard8)]

I've just reproduced this on my trunk Firefox build with mozconfig:

ac_add_options --enable-application=browser
ac_add_options --enable-debug
ac_add_options --disable-optimize
ac_add_options --enable-extensions=default,inspector
ac_add_options --disable-crashreporter
ac_add_options --enable-tests

Comment 2

[Mark Banner (:standard8)]

Further analysis in the debugger shows that:

- nsLayoutStatics::Initialize isn't being called
- nsLayoutModule::Initialize isn't being called

Hence this leads to the nsRegion::InitStatic not being called and hence the crash.

Comment 3

[Mark Banner (:standard8)]

Ok, this is libxul versus non-libxul. If I add the line:

  Components.classes["@mozilla.org/layout/xul-boxobject-tree;1"].createInstance(Components.interfaces.nsIBoxObject);

at the very start of the test, then it passes. That component is registered within nsLayoutModule, whereas the nsRegion ones aren't.

Comment 4

[Mark Banner (:standard8)]

Attachment: Possible fix

Possible fix by moving the initialisation from nsLayoutStatics to nsThebesGfxFactory - as nsThebesGfxFactory is what nsRegion is obtained from, it seems the better place to do it.

The possible downside to this is that it moves the destruction until after xpcom-shutdown, until the module is destructed - I've no idea if that is a problem or not.

I've pushed the patch to the try server anyway.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er... we have gfx xpcshell tests?  How does that work with other parts of gfx that are the responsibility of layout to initialize (like textrun stuff)?  That's what we modeled the region change on.

Which thread(s) are the module ctor/dtor called on?

Comment 6

[Mark Banner (:standard8)]

(In reply to comment #5)
> Er... we have gfx xpcshell tests?  How does that work with other parts of gfx
> that are the responsibility of layout to initialize (like textrun stuff)? 
> That's what we modeled the region change on.

Well it is fine if you're doing a libxul build - because everything is in the one library (afaict) which means if you load region, you get layout as well. That just doesn't happen for debug builds.

There is only this one xpcshell test in gfx at the moment which is the failing one - test_nsIScriptableRegion.js

> Which thread(s) are the module ctor/dtor called on?

Well afaik in xpcshell there is only one thread.

Comment 7

[Boris Zbarsky [:bzbarsky]]

I meant in the case that actually necessitated this change: multi-process Gecko-based apps.

Comment 8

[Mark Banner (:standard8)]

So what's the way forward on this. I guess the patch isn't what we want to do, so how about (for now at least) we do what I suggested in comment 3 and add:

> Components.classes["@mozilla.org/layout/xul-boxobject-tree;1"].createInstance(Components.interfaces.nsIBoxObject);

at the start of the test with an appropriate comment?

Then we can determine what to do with xpcshell-tests in gfx in slow time.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I would be fine with the comment 8 workaround while we sort this out, but I'm not a gfx peer.

Comment 10

[Mark Banner (:standard8)]

Attachment: [checked in] Workaround for test failure.

As discussed, this patch works around the test failures that we're seeing on the Thunderbird tinderboxes by making sure layout is loaded before gfx otherwise running this test will fail on all non-libxul builds.

If we take this patch, then I'll change this bug into a "fix initialisation of gfx & layout on non-libxul builds" bug.

Comment 11

[Mark Banner (:standard8)]

vlad: ping for review, I'd really like to be able to get the Thunderbird tinderboxes green again.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 400561 [details] [diff] [review]
[checked in] Workaround for test failure.

Checked in:
http://hg.mozilla.org/mozilla-central/rev/3c3c2efab7e8

Comment 13

[Mark Banner (:standard8)]

Changing summary to what I think we now want this bug to cover.

Comment 14

[Ed Morley [:emorley]]

I presume the workaround could be removed now that bug 638429 has landed. 

Is this worth me doing; or does it not hugely matter, given it's only for in a test?

Either way, presume this bug is now WONTFIX, if it only affects non-libxul builds?

Comment 15

[Robert Kaiser]

(In reply to Mark Banner (:standard8) from comment #3)
> Ok, this is libxul versus non-libxul.

As we don't support non-libxul any more, we should close it, then. Please REOPEN if you think this is not correct.

Comment 16

[Serge Gautherie (:sgautherie)]

Hum, R.Invalid doesn't seem right with comment 12 workaround still in the tree.
I assume it should be backed out if not needed anymore or its comment should be updated.

Comment 17

[Robert Kaiser]

No it is invalid. Please file another bug for removing the now apparently dead code checked in as a workaround here.

Comment 18

[Serge Gautherie (:sgautherie)]

(In reply to Robert Kaiser (:[e-mail]) from comment #17)
> No it is invalid. Please file another bug for removing the now apparently
> dead code checked in as a workaround here.

I'm not convinced, but I filed bug 707721...