888338 - GC: Post-barrier more browser heap based GC thing pointers

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jon Coppeard (:jonco)]

Searching for JS_Call*Tracer reveals there are a bunch of places where heap things are traced that we haven't converted to use post-barriers.

The main areas are:
 - workers
 - XPConnect
 - bindings

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hmm.  Where do you see an issue in bindings code here?

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Boris Zbarsky (:bz) (reading mail, but on paternity leave) from comment #1)
> Hmm.  Where do you see an issue in bindings code here?

For example, in WebComponentsBinding.cpp, ElementRegistrationOptions::TraceDictionary() calls JS_CallObjectTracer() to trace its mPrototype member.

Digging a little further, it seems TraceDictionary() is only called for stack rooting - are these classes never used on the heap then?

Comment 3

[Boris Zbarsky [:bzbarsky]]

TraceDictionary is indeed only called for stack rooting.  The dictionary itself might be on the heap, but only as part of a data structure that has stack lifetime and stack rooting.

Sadly, we have no way to statically enforce that.  :(

Comment 4

[Jon Coppeard (:jonco)]

Attachment: 1 - add trace method for objects we know are tenured

Comment 5

[Jon Coppeard (:jonco)]

Attachment: 2 - give post barrier callback function an extra data argument

Comment 6

[Jon Coppeard (:jonco)]

Attachment: 3 - don't call gray root tracer for minor GCs

Comment 7

[Jon Coppeard (:jonco)]

Attachment: 4 - barrier ctypes

Comment 8

[Jon Coppeard (:jonco)]

Attachment: 5 - a couple more barriers in XPConnect

Comment 9

[Jon Coppeard (:jonco)]

Attachment: 6 - barriers in the browser

Comment 10

[Jon Coppeard (:jonco)]

Attachment: 7 - barriers in workers

Comment 11

[Andrew McCreight [:mccr8]]

Olli or I are probably better reviewers for 6 and 7 as bz is on paternity leave.

Comment 12

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #11)

Thanks, I'll update.

Comment 13

[Terrence Cole [:terrence]]

Comment on attachment 771426 [details] [diff] [review]
2 - give post barrier callback function an extra data argument

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

r=me

::: js/src/gc/StoreBuffer.h
@@ +60,5 @@
>              return;
>          ValueType value = p->value;
>          JS_SET_TRACING_LOCATION(trc, (void*)&p->key);
>          Mark(trc, &key, "HashKeyRef");
> +        map->rekey(prior, key);

I've got a similar hunk in bug 888117. If you land first, also remove value above, and the definition of ValueType and Ptr, and switch &p->key to &*p so that this class can be used with HashSets too.

Comment 14

[Terrence Cole [:terrence]]

Comment on attachment 771428 [details] [diff] [review]
3 - don't call gray root tracer for minor GCs

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

r=me

Comment 15

[Terrence Cole [:terrence]]

Comment on attachment 771425 [details] [diff] [review]
1 - add trace method for objects we know are tenured

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

Instead of scattering assertions all over the place and adding a new API, could we instead make a new type TenuredHeap on <typename T, size_t Mask=0> that overrides the constructor and operator= to make the assertion? This would also allow us to override JS_CallHeapObjectTracer for the assertion at mark time and, I think, remove CallMaskedObjectTracer too. The only complexity is the Mask parameter to the template, but that shouldn't be a serious obstacle.

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 771430 [details] [diff] [review]
5 - a couple more barriers in XPConnect

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

I don't really know what this stuff does, but it looks reasonable.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +3407,5 @@
>      RemoveFromRootSet(nsXPConnect::GetRuntimeInstance()->GetMapLock());
>  }
>  
>  void
>  XPCJSObjectHolder::TraceJS(JSTracer *trc)

This is occasionally used on the stack, I believe. That should be ok, right?

Comment 17

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

Comment on attachment 771433 [details] [diff] [review]
7 - barriers in workers

>-  JS::Value mTimeoutVal;
>-  nsTArray<jsval> mExtraArgVals;
>+  JS::Heap<JS::Value> mTimeoutVal;
>+  nsTArray<JS::Heap<jsval> > mExtraArgVals;
While you're here, change jsval to JS::Value

>       if (!JS_CallFunctionValue(aCx, global, info->mTimeoutVal,
>                                 info->mExtraArgVals.Length(),
>-                                info->mExtraArgVals.Elements(), rval.address()) &&
>+                                info->mExtraArgVals.Elements()->unsafeGet(),
>+                                rval.address()) &&
Perhaps add a comment why unsafeGet is needed.

>     XMLHttpRequest::StateData state;
>-    // XXXbz there is no AutoValueRooter anymore?
>-    JS::AutoArrayRooter rooter(aCx, 1, &state.mResponse);
>+    JS::Rooted<JS::Value> response(aCx);
> 
>     state.mResponseTextResult = mResponseTextResult;
>     state.mResponseText = mResponseText;
> 
>     if (NS_SUCCEEDED(mResponseTextResult)) {
>       MOZ_ASSERT(JSVAL_IS_VOID(mResponse) || JSVAL_IS_NULL(mResponse));
>       state.mResponseResult = mResponseTextResult;
>-      state.mResponse = mResponse;
>+      response = mResponse;
>     }
>     else {
>       state.mResponseResult = mResponseResult;
> 
>       if (NS_SUCCEEDED(mResponseResult)) {
>         if (mResponseBuffer.data()) {
>           MOZ_ASSERT(JSVAL_IS_VOID(mResponse));
> 
>@@ -689,38 +688,35 @@ public:
>           JSStructuredCloneCallbacks* callbacks =
>             aWorkerPrivate->IsChromeWorker() ?
>             ChromeWorkerStructuredCloneCallbacks(false) :
>             WorkerStructuredCloneCallbacks(false);
> 
>           nsTArray<nsCOMPtr<nsISupports> > clonedObjects;
>           clonedObjects.SwapElements(mClonedObjects);
> 
>-          JS::Rooted<JS::Value> response(aCx);
>           if (!responseBuffer.read(aCx, response.address(), callbacks, &clonedObjects)) {
>             return false;
>           }
>-
>-          state.mResponse = response;
>         }
>         else {
>-          state.mResponse = mResponse;
>+          response = mResponse;
>         }
>       }
>     }
> 
>     state.mStatusResult = mStatusResult;
>     state.mStatus = mStatus;
> 
>     state.mStatusText = mStatusText;
> 
>     state.mReadyState = mReadyState;
> 
>     XMLHttpRequest* xhr = mProxy->mXMLHttpRequestPrivate;
>-    xhr->UpdateState(state);
>+    xhr->UpdateState(state, response);
> 
>     if (mUploadEvent && !xhr->GetUploadObjectNoCreate()) {
>       return true;
>     }
> 
>     JS::Rooted<JSString*> type(aCx, JS_NewUCStringCopyN(aCx, mType.get(), mType.Length()));
>     if (!type) {
>       return false;
>@@ -1401,36 +1397,36 @@ Proxy::HandleEvent(nsIDOMEvent* aEvent)
>   }
> 
>   return NS_OK;
> }
> 
> XMLHttpRequest::XMLHttpRequest(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> : XMLHttpRequestEventTarget(aCx), mJSObject(NULL), mUpload(NULL),
>   mWorkerPrivate(aWorkerPrivate),
>-  mResponseType(XMLHttpRequestResponseType::Text), mTimeout(0),
>-  mJSObjectRooted(false), mBackgroundRequest(false),
>+  mResponseType(XMLHttpRequestResponseType::Text), mResponse(JSVAL_VOID), mTimeout(0),
>+  mJSObjectRooted(nullptr), mBackgroundRequest(false),
>   mWithCredentials(false), mCanceled(false), mMozAnon(false), mMozSystem(false)
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
> }
> 
> XMLHttpRequest::~XMLHttpRequest()
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
>   MOZ_ASSERT(!mJSObjectRooted);
> }
> 
> void
> XMLHttpRequest::_trace(JSTracer* aTrc)
> {
>   if (mUpload) {
>-    mUpload->TraceJSObject(aTrc, "mUpload");
>+    mUpload->TraceJSObject(aTrc, "XMLHttpRequest::mUpload");
>   }
>-  JS_CallValueTracer(aTrc, &mStateData.mResponse, "mResponse");
>+  JS_CallHeapValueTracer(aTrc, &mResponse, "XMLHttpRequest::mResponse");
>   XMLHttpRequestEventTarget::_trace(aTrc);
> }
> 
> void
> XMLHttpRequest::_finalize(JSFreeOp* aFop)
> {
>   ReleaseProxy(XHRIsGoingAway);
>   XMLHttpRequestEventTarget::_finalize(aFop);
>@@ -1505,28 +1501,29 @@ XMLHttpRequest::MaybePin(ErrorResult& aR
>   mWorkerPrivate->AssertIsOnWorkerThread();
> 
>   if (mJSObjectRooted) {
>     return;
>   }
> 
>   JSContext* cx = GetJSContext();
> 
>-  if (!JS_AddNamedObjectRoot(cx, &mJSObject, "XMLHttpRequest mJSObject")) {
>+  mJSObjectRooted = mJSObject;
>+  if (!JS_AddNamedObjectRoot(cx, &mJSObjectRooted, "XMLHttpRequest::mJSObjectRooted")) {
>+    mJSObjectRooted = nullptr;
>     aRv.Throw(NS_ERROR_FAILURE);
>     return;
>   }
> 
>   if (!mWorkerPrivate->AddFeature(cx, this)) {
>-    JS_RemoveObjectRoot(cx, &mJSObject);
>+    JS_RemoveObjectRoot(cx, &mJSObjectRooted);
>+    mJSObjectRooted = nullptr;
>     aRv.Throw(NS_ERROR_FAILURE);
>     return;
>   }
>-
>-  mJSObjectRooted = true;
> }
> 
> void
> XMLHttpRequest::MaybeDispatchPrematureAbortEvents(ErrorResult& aRv)
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
>   MOZ_ASSERT(mProxy);
> 
>@@ -1630,21 +1627,21 @@ void
> XMLHttpRequest::Unpin()
> {
>   mWorkerPrivate->AssertIsOnWorkerThread();
> 
>   NS_ASSERTION(mJSObjectRooted, "Mismatched calls to Unpin!");
> 
>   JSContext* cx = GetJSContext();
> 
>-  JS_RemoveObjectRoot(cx, &mJSObject);
>+  JS_RemoveObjectRoot(cx, &mJSObjectRooted);
> 
>   mWorkerPrivate->RemoveFeature(cx, this);
> 
>-  mJSObjectRooted = false;
>+  mJSObjectRooted = nullptr;
> }
> 
> void
> XMLHttpRequest::SendInternal(const nsAString& aStringBody,
>                              JSAutoStructuredCloneBuffer& aBody,
>                              nsTArray<nsCOMPtr<nsISupports> >& aClonedObjects,
>                              ErrorResult& aRv)
> {
>@@ -2126,34 +2123,33 @@ XMLHttpRequest::SetResponseType(XMLHttpR
>   runnable->GetResponseType(acceptedResponseTypeString);
> 
>   mResponseType = ConvertStringToResponseType(acceptedResponseTypeString);
> }
> 
> jsval
> XMLHttpRequest::GetResponse(JSContext* /* unused */, ErrorResult& aRv)
> {
>-  if (NS_SUCCEEDED(mStateData.mResponseTextResult) &&
>-      JSVAL_IS_VOID(mStateData.mResponse)) {
>+  if (NS_SUCCEEDED(mStateData.mResponseTextResult) && JSVAL_IS_VOID(mResponse)) {
>     MOZ_ASSERT(mStateData.mResponseText.Length());
>     MOZ_ASSERT(NS_SUCCEEDED(mStateData.mResponseResult));
> 
>     JSString* str =
>       JS_NewUCStringCopyN(GetJSContext(), mStateData.mResponseText.get(),
>                           mStateData.mResponseText.Length());
>     if (!str) {
>       aRv.Throw(NS_ERROR_OUT_OF_MEMORY);
>       return JSVAL_VOID;
>     }
> 
>-    mStateData.mResponse = STRING_TO_JSVAL(str);
>+    mResponse = STRING_TO_JSVAL(str);
>   }
> 
>   aRv = mStateData.mResponseResult;
>-  return mStateData.mResponse;
>+  return mResponse;
> }

>-  bool mJSObjectRooted;
>+  JSObject* mJSObjectRooted;
So this should be now mRootedJSObject


>   void
>-  UpdateState(const StateData& aStateData)
>+  UpdateState(const StateData& aStateData, JS::Handle<JS::Value> aResponse)
>   {
>     mStateData = aStateData;
>+    mResponse = aResponse;
>   }
This is odd, since the method starts to do something else than just UpdateState.
The whole statedata is a bit odd, but could you keep all the mResponse* stuff there.

Comment 18

[Andrew McCreight [:mccr8]]

Comment on attachment 771431 [details] [diff] [review]
6 - barriers in the browser

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

::: dom/bindings/BindingUtils.h
@@ +273,5 @@
>  {
>    MOZ_ASSERT(js::GetObjectClass(obj)->flags & JSCLASS_DOM_GLOBAL);
>    MOZ_ASSERT(js::GetReservedSlot(obj, DOM_PROTOTYPE_SLOT).isUndefined());
>  
> +  JS::Heap<JSObject*>* protoAndIfaceArray = new JS::Heap<JSObject*>[kProtoAndIfaceCacheCount];

I guess JS::Heap always initializes to zero?

Comment 19

[Jon Coppeard (:jonco)]

Attachment: 1 - Add TenuredHeap<T> class

As per comments, I added a TenuredHeap<T> class for when we know the thing is always in the tenured heap.

This is better overall, but we still have to have the assertion for a couple of
cases where we need to trace a global that we've just got from calling the JS
API.

I hardcoded the number of mask bits because otherwise you end up with a
different type for each number of bits and have to overload the trace function
accordingly.

Comment 20

[Jon Coppeard (:jonco)]

Attachment: 2 - give post barrier callback function an extra data argument

Comment 21

[Jon Coppeard (:jonco)]

Attachment: 7 - barriers in workers

Updated following review comments.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

This touches some pretty central stuff in nsGlobalWindow and XPCWNs so somebody like maybe peterv or bz should review this part...

Comment 23

[Andrew McCreight [:mccr8]]

Also, you should document a bit more when somebody might want to use TenuredHeap.  Terrence said in IRC that things in the nursery can't use finalizers, which is presumably why you need this here.

Comment 24

[Terrence Cole [:terrence]]

Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

This is pretty much righteous. r=me, but I'd like to get a second reviewer for the mWrapper/mFlatJSObject changes. Those are extremely complicated. They appear right to me, but that code is complex enough that someone who really knows it should take a look.

::: content/base/src/nsContentUtils.cpp
@@ +1649,5 @@
>    if (!cx) {
>      return;
>    }
>    if (JSObject* global = js::GetDefaultGlobalForContext(cx)) {
> +    JS::AssertGCThingMustBeTenured(global);

Could we move this assertion into GetDefaultGlobalForContext?

::: dom/base/nsGlobalWindow.h
@@ +1224,5 @@
>    nsRefPtr<nsHistory>           mHistory;
>  
>    // These member variables are used on both inner and the outer windows.
>    nsCOMPtr<nsIPrincipal> mDocumentPrincipal;
> +  JS::TenuredHeap<JSObject*> mJSObject;

A comment explaining how we know that this is always tenured would be helpful.

::: js/public/RootingAPI.h
@@ +278,5 @@
> + * Additonally, we allow some flags to be stored in the lower bits of the
> + * pointer.
> + */
> +template <typename T>
> +class TenuredHeap : public js::HeapBase<T>

Olli and Andrew found this class quite confusing. They are okay with it with additional explanation, but they would like the comments on Heap and TenuredHeap and heap extended significantly.

Here is my take at a clearer comment on these classes. Olli, Andrew, Jon, what are your thoughts?

Heap
***********
The Heap<T> class is a C/C++ heap stored reference to a JS GC thing. GC references stored on the C/C++ stack must use Rooted/Handle/MutableHandle instead. Heap<T> wraps the complex mechanisms required to ensure GC safety for the contained reference into a C++ class that behaves similarly to a normal pointer.

Requirements for type T:
  - Must be one of: Value, jsid, JSObject*, JSString*
***********

TenuredHeap
***********
The TenuredHeap class is similar to the Heap class above in that it encapsulates the GC concerns of an on-heap reference to a JS object. However, it has two important differences:
  1) Pointers which are statically known to only reference "tenured" objects can avoid the extra overhead of SpiderMonkey's write barriers.
  2) Objects in the "tenured" heap have stronger alignment restrictions than those in the "nursery", so flag bits should only be stored on pointers known to be tenured. TenuredHeap wraps a normal tagged pointer with a nice API for accessing the flag bits and adds various assertions to ensure that it is not mis-used. 

GC things are said to be "tenured" when they are located in the long-lived heap: e.g. they have gained tenure as an object by surviving past at least one GC. For performance, SpiderMonkey allocates some things which are known to normally be long lived directly into the tenured generation; for example, global objects. Additionally, SpiderMonkey does not visit individual objects when deleting non-tenured objects, so object with finalizers are also always tenured; for instance, this includes most DOM objects.

The considerations to keep in mind when using a TenuredHeap<T> vs a normal Heap<T> are:
  - It is invalid for a TenuredHeap<T> to refer to a non-tenured thing.
  - It is invalid to store flag bits on a Heap<T>.
  - It is valid for Heap<T> to refer to a tenured thing.
***********

::: js/xpconnect/src/xpcprivate.h
@@ +1411,5 @@
>                          js::PointerHasher<JSObject *, 3>,
>                          js::SystemAllocPolicy> DOMExpandoSet;
>  
>      bool RegisterDOMExpandoObject(JSObject *expando) {
> +        JS::AssertGCThingMustBeTenured(expando);

A comment here explaining why all expandos are tenured would probably be helpful.

@@ +2475,1 @@
>          {

{ at end of previous line.

@@ +2578,5 @@
> +        WRAPPER_NEEDS_SOW = JS_BIT(0),
> +        WRAPPER_NEEDS_COW = JS_BIT(1),
> +        WRAPPER_DESTROYED = JS_BIT(2),
> +
> +        // Flags bits for mJSObject:

s/mJSObject/mFlatJSObject/

::: xpcom/base/CycleCollectedJSRuntime.cpp
@@ +517,5 @@
>        continue;
>      }
>  
>      if (JSObject* global = js::GetDefaultGlobalForContext(acx)) {
> +      JS::AssertGCThingMustBeTenured(global);

And remove this with the assertion moved into the getter.

Comment 25

[Terrence Cole [:terrence]]

Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

Ah, I see Andrew already added a second review. Thanks!

Comment 26

[Andrew McCreight [:mccr8]]

That seems a little wordy.  For instance, that Tenured can't hold non-Tenured things seems self-evident. ;)

Comment 27

[Jon Coppeard (:jonco)]

(In reply to Andrew McCreight [:mccr8] from comment #18)

> I guess JS::Heap always initializes to zero?

Yes, Heap<T> initializes to NULL when T is a pointer and to JS::UndefinedValue() when it is JS::Value.

Comment 28

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

Comment on attachment 772701 [details] [diff] [review]
7 - barriers in workers

>+  class StateDataAutoRooter : private JS::CustomAutoRooter
>+  {
>+  public:
>+    explicit StateDataAutoRooter(JSContext *cx, XMLHttpRequest::StateData *aData
JSContext* aCx, XMLHttpRequest::StateData* aData

>+                                 MOZ_GUARD_OBJECT_NOTIFIER_PARAM)
>+    : CustomAutoRooter(cx), mStateData(aData), mSkip(cx, mStateData)
>+    {
>+      MOZ_GUARD_OBJECT_NOTIFIER_INIT;
>+    }
>+
>+  private:
>+    virtual void trace(JSTracer *trc)
JSTracer* aTrc

>+    XMLHttpRequest::StateData *mStateData;
XMLHttpRequest::StateData* mStateData;

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 771429 [details] [diff] [review]
4 - barrier ctypes

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

r=me

Comment 30

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

Other than what mccr8 said, I think the comments are good.
If we make changes to Heap + Handle handling, need to add some comment about it, hopefully
some example even.

Comment 31

[Peter Van der Beken [:peterv]]

Comment on attachment 772698 [details] [diff] [review]
1 - Add TenuredHeap<T> class

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

::: dom/base/nsGlobalWindow.cpp
@@ +3148,5 @@
>  {
>    MOZ_ASSERT(IsOuterWindow());
>    if (aObject == mJSObject) {
> +    mJSObject = nullptr;
> +    mJSObject.setFlags(OUTER_WINDOW_PROXY_FINALIZED);

I don't understand how this gives us the same functionality as before, since we never seem to assert OUTER_WINDOW_PROXY_FINALIZED? Accessing mJSObject after PoisonOuterWindowProxy was called would crash, accessing it after OuterWindowProxyFinalized is called won't.

Comment 32

[Jon Coppeard (:jonco)]

(In reply to Peter Van der Beken [:peterv] from comment #31)

It looks like I missed the second part of the comment in nsOuterWindowProxy::finalize(), and only dealt with preventing EnsureScriptEnvironment() from doing anything when called after this.  

    // Ideally we would use OnFinalize here, but it's possible that
    // EnsureScriptEnvironment will later be called on the window, and we don't
    // want to create a new script object in that case. Therefore, we need to
    // write a non-null value that will reliably crash when dereferenced.

So if I assert that the OUTER_WINDOW_PROXY_FINALIZED is not set in methods that access mJSObject, would that be ok?

Comment 33

[Peter Van der Beken [:peterv]]

My main worry is that people will assume that null-checking is enough and not add a flag check. If they try to use it without null-checking then obviously we'll crash, so I'm not sure that adding OUTER_WINDOW_PROXY_FINALIZED checks everywhere is necessary. But not sure either what a good solution is, we could add a flag check in TenuredHeap, have a template argument to pass in the flag value to check for and add checks in the operators? Not sure it's worth it :-/.

Comment 34

[Jon Coppeard (:jonco)]

Attachment: 1 - Add TenuredHeap<T> class

In response to peterv's comment, I've changed this to add a method to TenuredHeap<T> to set the contents to a value that will cause a crash if it's dereferenced, so this now works basically the same as before.

Since it looks like he's away for the next few weeks, I'm asking bz to review.

Terrence has already reviewed the JS changes, see above.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 778515 [details] [diff] [review]
1 - Add TenuredHeap<T> class

Shouldn't XPCWrappedNative::Destroy use the crash-on-touch thing too?

>+        {if (mFlatJSObject)
>              xpc_UnmarkGrayObject(mFlatJSObject);

xpc_UnmarkGrayObject is null-safe, so no need for the null check.

r=me with those fixed

Comment 36

[Jon Coppeard (:jonco)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a80f35bdbaa0
https://hg.mozilla.org/integration/mozilla-inbound/rev/03f30c353078
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a1ecc394134
https://hg.mozilla.org/integration/mozilla-inbound/rev/f1ac09283bd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ba1235ccac
https://hg.mozilla.org/integration/mozilla-inbound/rev/a43fffe4e00c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8b1703dbd7ae

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a80f35bdbaa0
https://hg.mozilla.org/mozilla-central/rev/03f30c353078
https://hg.mozilla.org/mozilla-central/rev/7a1ecc394134
https://hg.mozilla.org/mozilla-central/rev/f1ac09283bd4
https://hg.mozilla.org/mozilla-central/rev/39ba1235ccac
https://hg.mozilla.org/mozilla-central/rev/a43fffe4e00c
https://hg.mozilla.org/mozilla-central/rev/8b1703dbd7ae