616262 - Remove cycle collection static initializers

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

Attachment: Group cycle collector global declarations

As of today, cycle collection is responsible for 180 static initializers out of 309. This is by far the main contender in the number of static initializers, yet, only one macro is involved to create them.

While it may not be a good time for intrusive changes such as those proposed in bug 569629, it is still possible to circumvent the issue while not fundamentally modifying the code. In fact, all we need to do is to move the cycle collector global variables declaration to a single place, which will get the number of associated static initializers down to 1, thus getting the total number of static initializers instantly down to 130, and slightly improving startup time.

This also has the added value of allowing better objects reordering in bug 603370 since all those object files originally containing these static initializers, but that are actually not used at startup, can now be put in the cold part of libxul.so. This should thus enhance the effects of bug 603370.

This should have a positive effect on both OSX and Linux. AFAIUI, Windows already packs static initializers so there's no benefit there, which is good, because the trick used in CycleCollectorGlobals.cpp doesn't work with MSVC. It is actually expected to only work with GCC.

I tried to put proper #ifdef on the various knobs that enable/disable different things in the code, but I can't guarantee this covers everything. At least, the main builds work, and build failures messages allow for easy fixes.

We'd really like that for 2.0.

(Tested on try, FWIW)

Comment 1

[Mike Hommey [:glandium]]

A question I have too, is where the fact that cycle collector globals also need to be defined in this need file should be documented?

Comment 2

[Mike Hommey [:glandium]]

(In reply to comment #1)
> A question I have too, is where the fact that cycle collector globals also need
> to be defined in this need file should be documented?

s/need/new/

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Presumably at https://developer.mozilla.org/en/interfacing_with_the_xpcom_cycle_collector

Comment 4

[Mike Hommey [:glandium]]

Attachment: Additional patch for new cycle collector globals as of today

This applies on top of the previous patch, and adds the cycle collector globals that were added to the codebase since the first patch.

Comment 5

[Benjamin Smedberg]

Comment on attachment 494801 [details] [diff] [review]
Group cycle collector global declarations

I'm having a kinda bad feeling about this, partly because of the weird GCC-specific class redefinitions. But I don't know enough about it to be certain, so I'm going to punt this over to peterv (or bent).

Comment 6

[Mike Hommey [:glandium]]

Since this is not going to make it for 4.0, let's use this bug to do it in a workaround form. This will probably mean a radical change in the cycle collection classes, though.

Comment 7

[Mike Hommey [:glandium]]

(In reply to comment #6)
> let's use this bug to do it in a workaround form.

"not" is missing here.

Comment 8

[Mike Hommey [:glandium]]

I would like to revisit this bug. As of today, cycle collector globals account for 220-something of the 360-something static initializers we have.
I see several different approach possible:
- Centralization of their declaration, like what is done in the (now outdated) patches here.
- Replacing their declaration with a hand-crafted declaration. That is, manually create a vtable (since the compiler is unable to give us a pointer to the one it creates :( ), and manually create the global object content (which is, a pointer to the vtable and the value of mMightSkip).
- Change the nsXPCOMCycleCollectionParticipant class and its parents so that they are function tables instead of classes with vtables.
Any opinion on the preferred approach? (I'd have a tendency to prefer the last one, but it makes the code less C++y)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

I think the last option should be ok if we don't regress performance and can hide the implementation
using the macros. Basically, can we do the last option by changing only the macros
and nsCycleCollectionParticipant classes?

Comment 10

[Mike Hommey [:glandium]]

(In reply to Olli Pettay [:smaug] from comment #9)
> I think the last option should be ok if we don't regress performance and can
> hide the implementation
> using the macros. Basically, can we do the last option by changing only the
> macros
> and nsCycleCollectionParticipant classes?

I think it's possible to mask it. And I don't think this would regress. In some cases, it would even be faster, as it would get rid of an indirection (reading the vtable pointer) in cases where the compiler don't optimize for that.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Add mozilla/TypeTraits.h with mozilla::Conditional and mozilla::IsBaseOf traits, similar to resp. C++11 std::conditional and std::is_base_of

Comment 12

[Mike Hommey [:glandium]]

Attachment: Avoid cycle collection participant global variables adding static initializers

Sent to try:
https://tbpl.mozilla.org/?tree=Try&rev=3f78e71b887e

Comments in the patch should make it self-explanatory.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #12)
> Sent to try:
> https://tbpl.mozilla.org/?tree=Try&rev=3f78e71b887e

Failure on Windows :( I'll bisect between my last working WIP patch and this one.

Comment 14

[Mike Hommey [:glandium]]

Comment on attachment 627726 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

Breakage comes from member function pointer snafu, thanks to MSVC implementing different sizes of such pointers. It also appears the member function pointer use is dangerous, even in gcc, but thankfully works because no descendant of nsCycleCollectionParticipant uses multiple-inheritance.

Comment 15

[Mike Hommey [:glandium]]

Attachment: Avoid cycle collection participant global variables adding static initializers

This completely removes the use of member function pointers. First, it made things difficult with MSVC because of the different sizes for such pointers (yay), and, even better, MSVC was actually creating static initializers to put the member function pointers in the function tables (yayÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂÃÂò).

So the result kind of sucks, but I have no better idea, short of drastically changing how Traverse calls Trace in the inherited case.

Comment 16

[Mike Hommey [:glandium]]

Forgot to mention, this is on trial here:
https://tbpl.mozilla.org/?tree=Try&rev=deee21a8f275

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 627724 [details] [diff] [review]
Add mozilla/TypeTraits.h with mozilla::Conditional and mozilla::IsBaseOf traits, similar to resp. C++11 std::conditional and std::is_base_of

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

::: mfbt/TypeTraits.h
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_TypeTraits_h_

Need a functionality overview comment here:

/* Template-based metaprogramming and type-testing facilities. */

Or somesuch.

@@ +10,5 @@
> +/*
> + * IsBaseOf allows to know whether a given class is derived from another.
> + *
> + * Consider the following class definitions:
> + *   class A {};

Blank line between code block and prose.

@@ +11,5 @@
> + * IsBaseOf allows to know whether a given class is derived from another.
> + *
> + * Consider the following class definitions:
> + *   class A {};
> + *   class B: public A {};

class B : public A (extra space)

@@ +18,5 @@
> + * mozilla::IsBaseOf<A, B>::value is true;
> + * mozilla::IsBaseOf<A, C>::value is false;
> + */
> +
> +template <class Base, class Derived>

No blank line before this.

@@ +19,5 @@
> + * mozilla::IsBaseOf<A, C>::value is false;
> + */
> +
> +template <class Base, class Derived>
> +class IsBaseOf

The contents of this class, and of the other structs, should be indented a further two spaces per mfbt/STYLE.  Two spaces' indent to the level for public/private modifiers (even if there are none), then a further two to actual members.

@@ +37,5 @@
> + */
> +template <bool condition, class A, class B>
> +struct Conditional
> +{
> +  typedef B type;

Types are InterCaps, so make that |typedef B Type;|.

@@ +41,5 @@
> +  typedef B type;
> +};
> +
> +template <class A, class B>
> +struct Conditional<true, A, B>

I think people will read true-case-first, false-case-second more naturally than the other way around as you have it, so let's make the specialization the |conditional = false| case and switch the typedefs.

Comment 18

[Mike Hommey [:glandium]]

Attachment: Add mozilla/TypeTraits.h with mozilla::Conditional and mozilla::IsBaseOf traits, similar to resp. C++11 std::conditional and std::is_base_of.

Addressed comment 17.

Comment 19

[Mike Hommey [:glandium]]

Attachment: Avoid cycle collection participant global variables adding static initializers

Refreshed against changes to the mfbt part.

Comment 20

[Mike Hommey [:glandium]]

Comment on attachment 629704 [details] [diff] [review]
Add mozilla/TypeTraits.h with mozilla::Conditional and mozilla::IsBaseOf traits, similar to resp. C++11 std::conditional and std::is_base_of.

Carrying over r+ from Waldo.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

This makes the code very hard to read :(

Comment 22

[Mike Hommey [:glandium]]

Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

Olli, Andrew and I all agree this would benefit from C++ template ninja eyes.

Comment 23

[Andrew McCreight [:mccr8]]

You only need to look at nsCycleCollectionParticipant.h.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

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

Well, this is kind of gross, but I don't know if it is all that much worse than what there is already there.  Thanks for fixing this.

::: js/xpconnect/src/nsXPConnect.cpp
@@ +542,5 @@
>          mCycleCollectionContext = nsnull;
>      return NS_OK;
>  }
>  
> +class nsXPConnectParticipant: public nsCycleCollectionParticipant

I like how you pulled this out of nsXPConnect. I've been considering doing that anyways.

::: xpcom/base/nsAgg.h
@@ +73,5 @@
>  {                                                                           \
>  public:                                                                     \
> +  static NS_METHOD UnlinkImpl(void *p);                                     \
> +  static NS_METHOD TraverseImpl(NS_CYCLE_COLLECTION_INNERCLASS *that,       \
> +                         void *p, nsCycleCollectionTraversalCallback &cb);  \

nit: Could you fix the indent here?  You can just put cb on its own line.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +120,5 @@
> + * The Traverse function may require calling another function from the cycle
> + * collection participant function table, so a pointer to the function table
> + * is given to it. Using member function pointers would be less awkward, but
> + * MSVC doesn't create such pointers of the same size depending on the
> + * subclass in which the function pointed to is defined, making it hard to

Nit: "MSVC doesn't create such pointers of the same size depending on the subclass in which the function pointed to is defined" is a little awkward to me. Maybe something like "in MSVC the size of such a function pointer depends on the class the function is defined in"?

@@ +123,5 @@
> + * MSVC doesn't create such pointers of the same size depending on the
> + * subclass in which the function pointed to is defined, making it hard to
> + * make them compatible with a generic function table. Moreover, static
> + * initialization of the function table then uses a static initializer
> + * function :(

Nit: No emoticon here please. :)

@@ +157,5 @@
> +{
> +    void (NS_STDCALL *Trace)(void *p, TraceCallback cb, void *closure);
> +};
> +
> +/* Additional functions for nsXPCOMCycleCollectionParticipantVTable */

Should this be "Additional functions for nsXPCOMCycleCollectionParticipant"?  As it is, it is inconsistent with the ScriptObjectTracerVtable.

@@ +280,3 @@
>  {
>  public:
> +    static void NS_COM_GLUE NoteChild(void *aScriptThing, const char *name, void *aClosure);

While you are touching this code anyways, could you rename NoteChild to NoteJSChild here and elsewhere?  Thanks.

@@ +878,2 @@
>    public:                                                                      \
> +    static NS_METHOD RootImpl(void *n);                                        \

Just as an aside, I'm guessing that most of these methods always return NS_OK.  If I wanted to, in a later patch, change them to be void functions instead of nsresult, I would change the NS_METHOD to NS_METHOD_(void)?

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

Somehow I managed to remove smaug r+ and replace it with an empty r?.  So just pretend it is still there.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Andrew McCreight [:mccr8] from comment #24)
> Just as an aside, I'm guessing that most of these methods always return
> NS_OK.  If I wanted to, in a later patch, change them to be void functions
> instead of nsresult, I would change the NS_METHOD to NS_METHOD_(void)?

Yes. And, like before, you would have to do that everywhere the function is defined (e.g. NS_DECL_CYCLE_COLLECTION_CLASS_NO_UNLINK, NS_DECL_CYCLE_COLLECTION_NATIVE_CLASS_BODY, NS_IMPL_CYCLE_COLLECTION_ROOT_NATIVE, nsXPConnect.cpp, etc.). The only additional requirement is to also change the type of the function pointer in nsCycleCollectionParticipantVTableCommon.

Please note that in the VTables, NS_METHOD is not used because NS_METHOD expands to NS_METHOD_(nsresult), which expands to nsresult __stdcall, which doesn't match the syntax for function pointers (nsresult (__stdcall *name)(args)).

Comment 27

[Jeff Walden [:Waldo]]

Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

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

"Gee, Officer Jeff, your eyes look glazed over.  Have you been eating donuts?"

Only looking at nsCycleCollectionParticipant.h, as noted earlier.

::: xpcom/glue/nsCycleCollectionParticipant.h
@@ +186,5 @@
> + *   };
> + */
> +template <typename T, bool isCycleCollectionParticipant,
> +                      bool isScriptObjectTracer,
> +                      bool isXPCOMCycleCollectionParticipant>

The specializations would be more readable if they used named enums for these rather than bools.

Also, as (constant) template parameters, perhaps these should be capitalized?

@@ +192,5 @@
> +
> +/* CCParticipantVTable for nsCycleCollectionParticipant */
> +template <typename T>
> +struct CCParticipantVTableImpl<T, true, false, false> {
> +    nsCycleCollectionParticipant *operator &()

You really really really shouldn't overload the address-of operator.  Why can't you use an |nsCycleCollectionParticipant* addr()| method?

@@ +201,5 @@
> +};
> +
> +/* CCParticipantVTable for nsScriptObjectTracer */
> +template <typename T>
> +struct CCParticipantVTableImpl<T, true, true, false> {

true, true, false?  No idea what these are without referring back to the original definition.  (Just to restate the readability concern at a place where it's important.)

@@ +226,5 @@
> +template <typename T>
> +struct CCParticipantVTable {
> +  typedef CCParticipantVTableImpl<T, mozilla::IsBaseOf<nsCycleCollectionParticipant, T>::value,
> +                                     mozilla::IsBaseOf<nsScriptObjectTracer, T>::value,
> +                                     mozilla::IsBaseOf<nsXPCOMCycleCollectionParticipant, T>::value>

Hmm, I guess the bool requirement would mean these would have to be ternaries of the enums.  More verbose?  Definitely.  More readable, enough to override the verbosity concern?  A closer call, but I still tend to think so.  This stuff is all awfully complex.  The next reader is going to appreciate every bit of readability you can possibly provide in all of this, seems to me.

@@ +825,5 @@
> +struct SkippableDummy
> +{
> +  static NS_METHOD_(bool) CanSkipImpl(void *p, bool aRemovingAllowed);
> +  static NS_METHOD_(bool) CanSkipInCCImpl(void *p);
> +  static NS_METHOD_(bool) CanSkipThisImpl(void *p);

Going solely from the comment, maybe you should should MOZ_DELETE these so that any inadvertent calls to them would be compile errors, if that's possible.

Comment 28

[Mike Hommey [:glandium]]

Attachment: Additional patch addressing review comments

This addresses Andrew's and Jeff's comments. As it's changing some cycle collection macros, I'm requesting another review from Olli.

As I don't want to inflict you another round of looking at the whole thing, this is only an interdiff. I'll fold when landing.

(In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #27)
> > + *   };
> > + */
> > +template <typename T, bool isCycleCollectionParticipant,
> > +                      bool isScriptObjectTracer,
> > +                      bool isXPCOMCycleCollectionParticipant>
> 
> The specializations would be more readable if they used named enums for
> these rather than bools.
> 
> Also, as (constant) template parameters, perhaps these should be capitalized?

Is the modified implementation what you had in mind?

> > +/* CCParticipantVTable for nsCycleCollectionParticipant */
> > +template <typename T>
> > +struct CCParticipantVTableImpl<T, true, false, false> {
> > +    nsCycleCollectionParticipant *operator &()
> 
> You really really really shouldn't overload the address-of operator.  Why
> can't you use an |nsCycleCollectionParticipant* addr()| method?

The reason is that I didn't want to cause more changes to other code, which I'm doing in this patch.

> > +struct SkippableDummy
> > +{
> > +  static NS_METHOD_(bool) CanSkipImpl(void *p, bool aRemovingAllowed);
> > +  static NS_METHOD_(bool) CanSkipInCCImpl(void *p);
> > +  static NS_METHOD_(bool) CanSkipThisImpl(void *p);
> 
> Going solely from the comment, maybe you should should MOZ_DELETE these so
> that any inadvertent calls to them would be compile errors, if that's
> possible.

Unfortunately, it doesn't work. At least GCC barfs because the functions are "used" (in an unreachable branch)

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 633069 [details] [diff] [review]
Additional patch addressing review comments


>-template <typename T, bool isCycleCollectionParticipant,
>-                      bool isScriptObjectTracer,
>-                      bool isXPCOMCycleCollectionParticipant>
>+
>+enum nsCycleCollectionParticipantType
>+{
>+  ISINVALID,
>+  ISCYCLECOLLECTIONPARTICIPANT,
>+  ISSCRIPTOBJECTTRACER,
>+  ISXPCOMCYCLECOLLECTIONPARTICIPANT
>+};
Coding style for enum values is usually eName.
So
enum nsCycleCollectionParticipantType
{
  eInvalid,
  eCycleCollectionParticipant,
  eScriptObjectTracer,
  eXPCOMCycleCollectionParticipant
};

Comment 30

[Jeff Walden [:Waldo]]

Comment on attachment 633069 [details] [diff] [review]
Additional patch addressing review comments

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

More precisely I was thinking something like this:

enum IsCCParticipant {
  CCParticipant,
  NotCCParticipant
};
enum IsScriptObjectTracer {
  ScriptObjectTracer,
  NotScriptObjectTracer
};

or something like that, but this seems to work about as well, too.

> Unfortunately, it doesn't work.

You could solve this problem with yet another level of template instantiation/indirection.  That said, I wouldn't be entirely surprised if some compiler doesn't barf on this as-is; I've had MSVC complain about undefined symbols before, and this is smelly in a similar way.

Comment 31

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/6a4feddc241b
https://hg.mozilla.org/mozilla-central/rev/560c492f81ad

Comment 32

[Mike Hommey [:glandium]]

Fixup for Win PGO.
https://hg.mozilla.org/mozilla-central/rev/a81526647059

This sucks, so I also filed bug 765159 to remove it.

Comment 33

[Mike Hommey [:glandium]]

Backed out because of what was filed as bug 765172:
https://hg.mozilla.org/mozilla-central/rev/005b54b174a3

I left the MFBT change in (attachment 629704 [details] [diff] [review])

Comment 34

[Andrew McCreight [:mccr8]]

This could require some adjustments from binary addons.  I'm not sure if that counts as addon compat or not.

Comment 35

[Mike Hommey [:glandium]]

(In reply to Andrew McCreight [:mccr8] from comment #34)
> This could require some adjustments from binary addons.  I'm not sure if
> that counts as addon compat or not.

Binary addons already need to be rebuilt for each version. The only adjustment that might be required is changing NS_IMPL_CYCLE_COLLECTION_CLASS definitions to either NS_IMPL_CYCLE_COLLECTION_NATIVE_CLASS or NS_IMPL_CYCLE_COLLECTION_SCRIPT_HOLDER_NATIVE_CLASS depending on whether it's defining a nsCycleCollectionParticipant or a nsScriptObjectTracer.

Comment 36

[Mike Hommey [:glandium]]

That being said, I doubt any addon implement Cycle collection for classes they provide. Heck, comm-central doesn't.

Comment 37

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Yeah, I would proceed under the assumption that all cycle collected classes are in Gecko.

Comment 38

[Andrew McCreight [:mccr8]]

Alrighty then.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

Not true. I've helped some addons devs to add support for CC.

Comment 40

[Mike Hommey [:glandium]]

(In reply to Olli Pettay [:smaug] from comment #39)
> Not true. I've helped some addons devs to add support for CC.

nsCycleCollectionParticipant, nsScriptObjectTracer or nsXPCOMCycleCollectionParticipant?

Comment 41

[Olli Pettay [:smaug][[e-mail]]]

nsScriptObjectTracer or nsXPCOMCycleCollectionParticipant via the macros, IIRC

Comment 42

[Peter Van der Beken [:peterv]]

Is the _NATIVE_ in the new macro names related to the _NATIVE_ in the existing macro names (where it means a refcounted non-XPCOM class)? If not this seems pretty confusing.

Comment 43

[Andrew McCreight [:mccr8]]

(In reply to Peter Van der Beken [:peterv] from comment #42)
> Is the _NATIVE_ in the new macro names related to the _NATIVE_ in the
> existing macro names (where it means a refcounted non-XPCOM class)?

Yup, same thing.  You just have to put _NATIVE in one additional place.

Comment 44

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #33)
> Backed out because of what was filed as bug 765172:
> https://hg.mozilla.org/mozilla-central/rev/005b54b174a3
> 
> I left the MFBT change in (attachment 629704 [details] [diff] [review])

Here's a crazy fact: there are exactly 4 changesets which lead to a green Moth with these patches:
8cf5423b0213, b30e903d23a0, 7625b37383fe and 3f408698a03f. Well, in fact I don't know for the middle two, i just know that the first and the last are green, and the the one preceding the first and the one following the last are both orange. I was just extremely lucky that it wasn't orange when I tested on try.

Comment 45

[Mike Hommey [:glandium]]

Gah, I didn't mean to reclose.

Comment 46

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a112cf3d055d

Comment 47

[Ed Morley [:emorley]]

[leave open] has to be in the whiteboard, or the new tool will close the bug:
https://groups.google.com/d/topic/mozilla.dev.platform/DQyQ56DgmhA/discussion

Comment 48

[Mike Hommey [:glandium]]

https://hg.mozilla.org/mozilla-central/rev/83369c1bb9af