Remove cycle collection static initializers

RESOLVED FIXED in mozilla16

Status

()

Core
Build Config
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

(Blocks: 1 bug, {addon-compat})

Trunk
mozilla16
addon-compat
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

7 years ago
Created attachment 494801 [details] [diff] [review]
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)
Attachment #494801 - Flags: review?(benjamin)
(Assignee)

Comment 1

7 years ago
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?
(Assignee)

Comment 2

7 years ago
(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/
Presumably at https://developer.mozilla.org/en/interfacing_with_the_xpcom_cycle_collector
Assignee: nobody → mh+mozilla
(Assignee)

Comment 4

7 years ago
Created attachment 495833 [details] [diff] [review]
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.
Attachment #495833 - Flags: review?(benjamin)
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).
Attachment #494801 - Flags: review?(benjamin) → review?(peterv)
Attachment #495833 - Flags: review?(benjamin) → review?(peterv)
(Assignee)

Updated

7 years ago
Attachment #494801 - Flags: review?(peterv)
(Assignee)

Updated

7 years ago
Attachment #495833 - Flags: review?(peterv)
(Assignee)

Comment 6

7 years ago
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.
Summary: Reduce cycle collection static initializers → Remove cycle collection static initializers
(Assignee)

Comment 7

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

"not" is missing here.
(Assignee)

Comment 8

5 years ago
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

5 years ago
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?
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Updated

5 years ago
Attachment #494801 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #495833 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Created 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
Attachment #627724 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 12

5 years ago
Created attachment 627726 [details] [diff] [review]
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.
Attachment #627726 - Flags: review?(bugs)

Updated

5 years ago
Attachment #627726 - Flags: review?(continuation)
(Assignee)

Comment 13

5 years ago
(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.
(Assignee)

Comment 14

5 years ago
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.
Attachment #627726 - Flags: review?(continuation)
Attachment #627726 - Flags: review?(bugs)
(Assignee)

Comment 15

5 years ago
Created attachment 628308 [details] [diff] [review]
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.
Attachment #628308 - Flags: review?(continuation)
Attachment #628308 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #627726 - Attachment is obsolete: true
(Assignee)

Comment 16

5 years ago
Forgot to mention, this is on trial here:
https://tbpl.mozilla.org/?tree=Try&rev=deee21a8f275
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.
Attachment #627724 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 18

5 years ago
Created 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.

Addressed comment 17.
(Assignee)

Updated

5 years ago
Attachment #627724 - Attachment is obsolete: true
(Assignee)

Comment 19

5 years ago
Created attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

Refreshed against changes to the mfbt part.
Attachment #629705 - Flags: review?(continuation)
Attachment #629705 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #628308 - Attachment is obsolete: true
Attachment #628308 - Flags: review?(continuation)
Attachment #628308 - Flags: review?(bugs)
(Assignee)

Comment 20

5 years ago
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.
Attachment #629704 - Flags: review+
Comment on attachment 629705 [details] [diff] [review]
Avoid cycle collection participant global variables adding static initializers

This makes the code very hard to read :(
Attachment #629705 - Flags: review?(bugs) → review+
(Assignee)

Comment 22

5 years ago
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.
Attachment #629705 - Flags: review?(jwalden+bmo)
You only need to look at nsCycleCollectionParticipant.h.
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)?
Attachment #629705 - Flags: review?(continuation)
Attachment #629705 - Flags: review?
Attachment #629705 - Flags: review+
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.
Attachment #629705 - Flags: review?

Updated

5 years ago
Attachment #629705 - Flags: review+
(Assignee)

Comment 26

5 years ago
(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 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.
Attachment #629705 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 28

5 years ago
Created attachment 633069 [details] [diff] [review]
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)
Attachment #633069 - Flags: review?(jwalden+bmo)
Attachment #633069 - Flags: review?(bugs)
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
};
Attachment #633069 - Flags: review?(bugs) → review+
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.
Attachment #633069 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 31

5 years ago
https://hg.mozilla.org/mozilla-central/rev/6a4feddc241b
https://hg.mozilla.org/mozilla-central/rev/560c492f81ad
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
(Assignee)

Updated

5 years ago
Depends on: 765159
(Assignee)

Comment 32

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

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

Updated

5 years ago
Depends on: 765172
(Assignee)

Comment 33

5 years ago
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])
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This could require some adjustments from binary addons.  I'm not sure if that counts as addon compat or not.
Keywords: addon-compat
(Assignee)

Comment 35

5 years ago
(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.
(Assignee)

Comment 36

5 years ago
That being said, I doubt any addon implement Cycle collection for classes they provide. Heck, comm-central doesn't.
Yeah, I would proceed under the assumption that all cycle collected classes are in Gecko.
Alrighty then.
Keywords: addon-compat
Not true. I've helped some addons devs to add support for CC.
Keywords: addon-compat
(Assignee)

Comment 40

5 years ago
(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?
nsScriptObjectTracer or nsXPCOMCycleCollectionParticipant via the macros, IIRC
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.
(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.
(Assignee)

Comment 44

5 years ago
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 45

5 years ago
Gah, I didn't mean to reclose.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/a112cf3d055d
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
[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
Whiteboard: [leave open]
(Assignee)

Comment 48

5 years ago
https://hg.mozilla.org/mozilla-central/rev/83369c1bb9af
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Updated

5 years ago
Whiteboard: [leave open]
(Assignee)

Updated

5 years ago
Depends on: 775424

Updated

4 years ago
Duplicate of this bug: 590937
(Assignee)

Updated

4 years ago
Depends on: 881323
You need to log in before you can comment on or make changes to this bug.