577500 - TestAtoms and TestStaticAtoms failed due to not being able to add static atoms after component registration (when run from dist/bin)

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect)

Description

[Mark Banner (:standard8)]

Since the component registration manager changes, TestStaticAtoms has been failing. It is only run on non-libxul builds which is why Firefox tree hasn't picked it up - but it is seen on the Thunderbird trees on all platforms.

The failure:

Running TestStaticAtoms tests...
TEST-PASS | foo is a static atom
TEST-PASS | foo is the right pointer
TEST-UNEXPECTED-FAIL | Did not get a static atom for foo
Finished running TestStaticAtoms tests.

I've verified this on both Thunderbird and Firefox builds, and I've tracked the regression range to the merge of the component manager changes.

I can't see anything obvious that has changed, but I might have missed something.

Comment 1

[Robert Kaiser]

The SeaMonkey trees just came back to building successfully and are failing with this on all platforms as well.

Comment 2

[Mark Banner (:standard8)]

Attachment: Disable TestStaticAtoms.cp

I've done some debugging, and here's the analysis:

- TestStaticAtoms only runs on non-libxul builds (hence why Firefox doesn't currently see failures on Tinderbox).

- If I run TestStaticAtoms from objdir/xpcom/tests it passes.

- If I run TestStaticAtoms from objdir/dist/bin it fails with the error shown in comment 0.

Further debugging shows that when run from objdir/dist/bin:

- ScopedXPCOM startup is loading in manifests
- as part of this, it loads in and initialises nsLayoutStatics
- At the end of nsLayoutStatics::Initialize, NS_SealStaticAtomTable is called
- The test then tries to add more static atoms, but they don't actually get to being added to the static atom table.

Therefore I'm sure that this is all fallout from the xpcom component registration changes.

I don't see a way where we can make this test work, and given that Firefox doesn't run it by default, I'm proposing that we disable it so we can clear the bustage on non-Firefox trees, but leave the bug open and if someone wants to find a way to fix it then they can.

Although the patch is just disabling the test, requesting review/approval from Benjamin in case he knows or can think of something I've missed to easily fix this.

Comment 3

[[e-mail]]

(In reply to comment #2)
> - ScopedXPCOM startup is loading in manifests
> - as part of this, it loads in and initialises nsLayoutStatics
XPCOM startup initialises layout statics? That sounds bad.

Comment 4

[Benjamin Smedberg]

Yes, xpcom startup register chrome packages, which gets xpconnect, which initializes layout. Why does that sound bad?

Comment 5

[[e-mail]]

Ah, I didn't realise that initialising all sorts of stuff very early on was an expected feature of the new XPCOM registration.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 457285 [details] [diff] [review]
Disable TestStaticAtoms.cp

I spoke to Benjamin over irc and he didn't have a better suggestion, so I've disabled this test for now. We'll leave this bug open to investigate fixing and re-enabling of the test.

Comment 7

[Mark Banner (:standard8)]

This was the disabling of TestStaticAtoms:

http://hg.mozilla.org/mozilla-central/rev/bd21c5390faf

I've also had to disable part of TestAtoms which was also testing static atoms, and permanently failing on Windows:

http://hg.mozilla.org/mozilla-central/rev/dfa2e299cb3e

(again a non-libxul only test).

Comment 8

[Mark Banner (:standard8)]

Bug 1333135 removed TestStaticAtoms, TestAtoms looks to have been moved to a gtest, and is running there without commented out parts.

-> WFM