1224596 - Stop messing about with JSContexts in WorkerMainThreadRunnable::Dispatch

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The current setup is all weird.  Some callers just leave the exception (if any) dangling.  Some report it immediately instead of propagating to caller (why?).

My plan is as follows:

1)  Introduce a version of Dispatch() that takes an ErrorResult instead of a JSContext.

2)  Convert all consumers to using it.  They will then probably all reliably throw the exception to caller.

3)  Burn the JSContext version with fire.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a version of WorkerMainThreadRunnable::Dispatch that takes an ErrorResult to report failure to dispatch on

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Switch Navigator to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make some WebSocket(Impl) methods whose return value is totally ignored void

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch WebSocket to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Switch nsPerformance to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Switch BroadcastChannel to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Switch ImageBitmap to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Switch Notification to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Switch DataStore to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Switch DataStoreCursor to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Switch WorkerNavigator to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Switch ServiceWorkerRegistration to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Switch gfxUtils to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Switch URL to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Remove the old WorkerMainThreadRunnable::Dispatch signature

Comment 17

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

Woohoo!

Comment 18

[Boris Zbarsky [:bzbarsky]]

> They will then probably all reliably throw the exception to caller.

I'm such an optimist.  Some of this code has nowhere to get even close to throwing.  :(  But at least now it could try if it wanted to....

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8687486 [details] [diff] [review]
part 14.  Switch URL to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/URL.cpp
@@ +419,5 @@
>    : WorkerMainThreadRunnable(aWorkerPrivate)
>    , mValue(aValue)
>    , mType(aType)
>    , mURLProxy(aURLProxy)
> +  , mFailed(false)

This means that we don't propagate the correct ErrorResult. What about if we use nsresult here and we set it back to the original ErrorResult?

Maybe we can do:

class BaseSetterGetterRunnable : public WorkerMainThreadRunnable
{
public:
  BaseSetterGetterRunnable(ErrorResult& aRv) : mRv(aRv) {}

  bool Dispatch() {
    if (WorkerMainThreadRunnable::Dispatch() && NS_FAILED(mNSResult)) {
      mRv.Throw(mNSResult);
    }
  }

private:
  void SetNSResult(nsresult aRv) { mNSResult = aRv; }
}

Then, all the setter and getter classes can just do: SetNSResult(localRv.StealNSResult) and the rest of the code is untouched.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> This means that we don't propagate the correct ErrorResult.

Yes, but does it matter?  The only case we propagate it for is SetHref (both before and after the patch).  The only errors it throws are:

1)  Failure to get the nsIIOService.  This throws whatever nsresult GetService returned, but honestly, that's just not going to happen on this codepath: if we couldn't create an ioservice, we wouldn't even _have_ a worker.

2)  Failure to do NewURI.  In this case, the result returned by NewURI is ignored and the code instead does ThrowTypeError<MSG_INVALID_URL>, which is exactly what I did in SetHref if we come back with a failure.

> What about if we use nsresult here and we set it back to the original ErrorResult?

That won't work for ErrorResults which had ThrowTypeError, or in fact anything other than the basic Throw() called on them.

> Then, all the setter and getter classes can just do:
> SetNSResult(localRv.StealNSResult) and the rest of the code is untouched.

I'm not sure what you're suggesting here.  Again the only getter or setter on URL that can throw per spec is the "href" setter.  That's what we currently implement for the worker URL, afaict.

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8687486 [details] [diff] [review]
part 14.  Switch URL to using the new WorkerMainThreadRunnable::Dispatch signature

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

Thanks for you comment.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8687487 [details] [diff] [review]
part 16.  Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

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

I'm sorry, but I'm not sure what we really get from propagating this boolean failure back.  It doesn't seem to be doing anything useful to me.  Can you explain what its actually causing to be cleaned up or shutdown?  I'm not seeing it.

Thanks.

::: dom/fetch/Fetch.cpp
@@ +548,5 @@
>  
>    bool
>    WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate) override
>    {
> +    return mFetchBody->ContinueConsumeBody(mStatus, mLength, mResult);

What do we really gain from returning false from WorkerRun()?  I guess WorkerRunnable::PostRun() calls JS_ReportPendingException() in this case?  I don't think we have any pending exceptions from the dispatch failure, though.

@@ +620,5 @@
>          }
>        } else {
>          mBody->ContinueConsumeBody(NS_ERROR_FAILURE, 0, nullptr);
> +        // XXXbz And if that fails and we're supposed to bail out of
> +        // everything, then what?

You can use MOZ_ALWAYS_TRUE() here.  This code is AssertIsOnMainThread(), while ContinueConsumeBody() only ever returns false on the worker thread.

@@ +755,5 @@
>    {
>      MOZ_ASSERT(aStatus > workers::Running);
>      if (!mWasNotified) {
>        mWasNotified = true;
> +      return mBody->ContinueConsumeBody(NS_BINDING_ABORTED, 0, nullptr);

Returning false here does nothing other than triggering a warning statement:

  https://dxr.mozilla.org/mozilla-central/source/dom/workers/WorkerPrivate.cpp#5163

Since we already get a warning in ContinueConsumeBody() we probably don't need that.  I think we should always just return true.

@@ +987,3 @@
>            NS_WARNING("Could not dispatch CancelPumpRunnable. Nothing we can do here");
> +          // XXXbz We're really supposed to totally bail out of everything
> +          // here... Ah, well.

Is there anything to bail out of if the main thread is gone?  Its not like the pump is going to continue to run...

@@ +987,5 @@
>            NS_WARNING("Could not dispatch CancelPumpRunnable. Nothing we can do here");
> +          // XXXbz We're really supposed to totally bail out of everything
> +          // here... Ah, well.
> +          localPromise->MaybeReject(rv);
> +          return false;

I'm personally not sure its worth propagating this boolean back.  Its generally not used for anything useful.  To the casual reader, however, it appears significant.  I think it would be better to just not return anything here and avoid confusion.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> but I'm not sure what we really get from propagating this boolean failure back.

Ben (Turner) told me the contract for WorkerRun (and in fact pretty much everything in workers that takes a JSContext argument) is that if you hit a syncloop failure under it that means the worker has been shut down and you have to stop everything and return false until you get back to the main worker event loop.  This code is clearly violating that contract, and I don't claim that after my changes it _follows_ it, but it's a bit closer.

> I don't think we have any pending exceptions from the dispatch failure, though.

You totally do in today's world if the actual dispatch to mainthread fails.  If the syncloop fails, you're supposed to act like an uncatchable exception and stop doing everything, per above.

> You can use MOZ_ALWAYS_TRUE() here.

OK.

> Is there anything to bail out of if the main thread is gone?

I don't know.  I'm not particularly familiar with this code, just trying to make it obey the invariants I'm told these things are supposed to obey.

I'm happy to just eat the exception instead if you think that's the right thing in ContinueConsumeBody (rv.SuppressException() and move on with life).  But,again, that's not what I'm told worker code expects to happen in general.

Comment 24

[Boris Zbarsky [:bzbarsky]]

OK, talked to bkelly.  Summary:

1)  The return value of WorkerRun only affects one thing: what gets passed to PostRun.  These runnables in Fetch don't override PostRun, so it only matters if there is an exception on the JSContext.  We know there isn't one.  So no reason to return false from WorkerRun.

2)  As bkelly points out, in ~AutoFailConsumeBody we're on the main thread so don't reach this runnable codepath at all.

3)  FetchBodyFeature::Notify is an override of WorkerFeature::Notify.  Looks like the only caller is WorkerPrivate::NotifyFeatures which basically ignores the return value.

Given all that, going to change ContinueConsumeBody back to just returning void and doing SuppressException.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

Comment 26

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8688206 [details] [diff] [review]
part 16.  Switch Fetch to using the new WorkerMainThreadRunnable::Dispatch signature

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

Thanks!

Comment 27

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

Comment on attachment 8687473 [details] [diff] [review]
part 1.  Add a version of WorkerMainThreadRunnable::Dispatch that takes an ErrorResult to report failure to dispatch on

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

::: dom/workers/WorkerRunnable.cpp
@@ +589,5 @@
> +
> +  if (NS_FAILED(NS_DispatchToMainThread(runnable.forget(), NS_DISPATCH_NORMAL))) {
> +    aRv.ThrowTypeError<MSG_NO_MAINTHREAD_DISPATCH>();
> +    return;
> +  }

Based on my reading of the code in xpcom/threads, I think it's impossible for this to fail at least through xpcom-shutdown-threads. And all worker threads are joined then.

If you agree, I would like to just assert that here.

@@ +595,5 @@
> +  if (!syncLoop.Run()) {
> +    // XXXbz or should this just throw an uncatchable exception instead?  That
> +    // way we could even throw it from promise-returning methods...
> +    aRv.ThrowTypeError<MSG_WORKER_SHUTTING_DOWN>();
> +  }

Yes, it should probably throw an uncatchable exception. There are probably some places where we return false where that's overkill though. e.g. http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133

Comment 28

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

Comment on attachment 8687476 [details] [diff] [review]
part 4.  Switch WebSocket to using the new WorkerMainThreadRunnable::Dispatch signature

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

These changes look fine. If you want answers to your questions you should probably ask baku or smaug.

Comment 29

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

Comment on attachment 8687478 [details] [diff] [review]
part 6.  Switch BroadcastChannel to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/broadcastchannel/BroadcastChannel.cpp
@@ +387,5 @@
>  
>      RefPtr<InitializeRunnable> runnable =
>        new InitializeRunnable(workerPrivate, origin, principalInfo,
>                               privateBrowsing, aRv);
> +    runnable->Dispatch(aRv);

Hmm shouldn't this also return null if dispatch fails (and aRv is never set by InitializeRunnable)?

Comment 30

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

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #29)
> Comment on attachment 8687478 [details] [diff] [review]
> part 6.  Switch BroadcastChannel to using the new
> WorkerMainThreadRunnable::Dispatch signature
> 
> Review of attachment 8687478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/broadcastchannel/BroadcastChannel.cpp
> @@ +387,5 @@
> >  
> >      RefPtr<InitializeRunnable> runnable =
> >        new InitializeRunnable(workerPrivate, origin, principalInfo,
> >                               privateBrowsing, aRv);
> > +    runnable->Dispatch(aRv);
> 
> Hmm shouldn't this also return null if dispatch fails (and aRv is never set
> by InitializeRunnable)?

Er, ignore that, I got confused.

Comment 31

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

Comment on attachment 8687479 [details] [diff] [review]
part 7.  Switch ImageBitmap to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/canvas/ImageBitmap.cpp
@@ +1069,5 @@
>      ErrorResult rv;
>      RefPtr<DecodeBlobInMainThreadSyncTask> task =
>        new DecodeBlobInMainThreadSyncTask(mWorkerPrivate, *mBlob, mCropRect,
>                                           rv, getter_AddRefs(data));
> +    task->Dispatch(rv); // This is a synchronous call.

This runnable returns false if rv gets set, which is bogus. Want to fix that too?

Comment 32

[Boris Zbarsky [:bzbarsky]]

> I think it's impossible for this to fail at least through xpcom-shutdown-threads.

Hmm.  So this can fail if nsThreadManager::Shutdown has been called, which certainly happens after xpcom-shutdown-threads.

It can also fail if nsThread::Dispatch fails, which can only happen if DispatchInternal called with a null nsNestedEventTarget* fails.  That can fail if:

1) The runnable is null.  Let's assume we don't do that.
2) gXPCOMThreadsShutDown.  This is only set after xpcom-shutdown-threads.
3) Some cases in the DISPATCH_SYNC stuff, not relevant here.
4) PutEvent() fails.  This can only happen if queue is null or if mEventsAreDoomed.  Since
   aTarget is null, queue can't be null.  mEventsAreDoomed gets set once we have exited
   MessageLoop::Run in ThreadFunc.  As far as I can tell, we never call ThreadFunc on the
   main thread, so we never set mEventsAreDoomed there.

Given that analysis, I agree with your claim.  Nathan thinks we should be setting mEventsAreDoomed _sometime_ on the main thread, though.  I assume if we do it will be after xpcom-shutdown-threads.  I'll change to asserting.

> Yes, it should probably throw an uncatchable exception.

OK, done.  

> There are probably some places where we return false where that's overkill though. e.g.
> http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133

Seems to me like throwing uncatchable exception there is fine.  If we're going away, we want to just stop the JS that's calling the URL constructor and go away.

> This runnable returns false if rv gets set, which is bogus. Want to fix that too?

You mean in MainThreadRun?  Done.  What _are_ the semantics of the return value of MainThreadRun, exactly?

Comment 33

[Boris Zbarsky [:bzbarsky]]

> You mean in MainThreadRun?  Done. 

On, and was comment 31 "r+ if you fix this one return value", or are there other issues?

Comment 34

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

(In reply to Boris Zbarsky [:bz] from comment #32)
> > I think it's impossible for this to fail at least through xpcom-shutdown-threads.
> 
> Hmm.  So this can fail if nsThreadManager::Shutdown has been called, which
> certainly happens after xpcom-shutdown-threads.
> 
> It can also fail if nsThread::Dispatch fails, which can only happen if
> DispatchInternal called with a null nsNestedEventTarget* fails.  That can
> fail if:
> 
> 1) The runnable is null.  Let's assume we don't do that.
> 2) gXPCOMThreadsShutDown.  This is only set after xpcom-shutdown-threads.
> 3) Some cases in the DISPATCH_SYNC stuff, not relevant here.
> 4) PutEvent() fails.  This can only happen if queue is null or if
> mEventsAreDoomed.  Since
>    aTarget is null, queue can't be null.  mEventsAreDoomed gets set once we
> have exited
>    MessageLoop::Run in ThreadFunc.  As far as I can tell, we never call
> ThreadFunc on the
>    main thread, so we never set mEventsAreDoomed there.
> 
> Given that analysis, I agree with your claim.  Nathan thinks we should be
> setting mEventsAreDoomed _sometime_ on the main thread, though.  I assume if
> we do it will be after xpcom-shutdown-threads.  I'll change to asserting.

I'm 99.9% sure Nathan is wrong. The only place I can see where we set it is at http://mxr.mozilla.org/mozilla-central/source/xpcom/threads/nsThread.cpp#401, and on line 372 it's pretty clear we're dealing with non-main-threads here.

> > Yes, it should probably throw an uncatchable exception.
> 
> OK, done.  
> 
> > There are probably some places where we return false where that's overkill though. e.g.
> > http://mxr.mozilla.org/mozilla-central/source/dom/workers/URL.cpp#133
> 
> Seems to me like throwing uncatchable exception there is fine.  If we're
> going away, we want to just stop the JS that's calling the URL constructor
> and go away.

Perhaps this is a bad example since it really shouldn't happen in practice, but what about taking that early return suggests that we're going away.

> > This runnable returns false if rv gets set, which is bogus. Want to fix that too?
> 
> You mean in MainThreadRun?  Done.  What _are_ the semantics of the return
> value of MainThreadRun, exactly?

That return value gets propogated to be the return value of AutoSyncLoopHolder::Run.

Comment 35

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

(In reply to Boris Zbarsky [:bz] from comment #33)
> > You mean in MainThreadRun?  Done. 
> 
> On, and was comment 31 "r+ if you fix this one return value", or are there
> other issues?

Yes. I don't see any other problems.

Comment 36

[Boris Zbarsky [:bzbarsky]]

> I'm 99.9% sure Nathan is wrong

Sorry, I was unclear.  He agrees we're not ever setting that boolean to true on the main thread right now.  He thinks we should change to doing so.

> but what about taking that early return suggests that we're going away.

Oh, I was talking specifically about throwing an uncatchable exception if the syncloop call in runnable dispatch returns false.  As I understand, _that_ can only happen if we're going away.  Is that not the case?

> That return value gets propogated to be the return value of AutoSyncLoopHolder::Run.

I see.  I'll try to add some docs for that.

> Yes. I don't see any other problems.

OK, great.  I made that change.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Is that not the case?

I guess given what you said about MainThreadRun's return value, false return from syncloop really doesn't mean so much....

Comment 38

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

(In reply to Boris Zbarsky [:bz] from comment #37)
> > Is that not the case?
> 
> I guess given what you said about MainThreadRun's return value, false return
> from syncloop really doesn't mean so much....

It should only be used in cases where we want to kill the running script in the worker. Those should be quite rare.

Comment 39

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

Comment on attachment 8687480 [details] [diff] [review]
part 8.  Switch Notification to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/notification/Notification.cpp
@@ +2193,5 @@
> +    r->Dispatch(rv);
> +    // XXXbz I'm told throwing and returning false from here is pointless (and
> +    // also that doing sync stuff from here is really weird), so I guess we just
> +    // suppress the exception on rv, if any.
> +    rv.SuppressException();

Yeah, this is kind of fucked.  Can you file a bug on making this not-sync?

Comment 40

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

Comment on attachment 8687483 [details] [diff] [review]
part 11.  Switch WorkerNavigator to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/Navigator.cpp
@@ +137,5 @@
>                                              eventProxy);
> +    ErrorResult rv;
> +    runnable->Dispatch(rv);
> +    if (rv.Failed()) {
> +      ThrowMethodFailed(aCx, rv);

What is ThrowMethodFailed? And why can't I find it in MXR?

Comment 41

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

For comment 40.

Comment 42

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

Comment on attachment 8687484 [details] [diff] [review]
part 12.  Switch ServiceWorkerRegistration to using the new WorkerMainThreadRunnable::Dispatch signature

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

::: dom/workers/ServiceWorkerRegistration.cpp
@@ +1114,1 @@
>      }

This gets called from Notify, so in practice I don't this ever happens.  Suppressing rv seems fine to me.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> Yeah, this is kind of fucked.  Can you file a bug on making this not-sync?

Bug 1227397 filed.

> What is ThrowMethodFailed? And why can't I find it in MXR?

It used to be the way to take an ErrorResult and throw it on a JSContext.

You can't find it because it got removed in bug 1224007 a few days ago.  The corresponding bit in the merge I have locally is now:

+    ErrorResult rv;
+    runnable->Dispatch(rv);
+    if (rv.MaybeSetPendingException(aCx)) {
+      return nullptr;
+    }

Comment 44

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/05254ae81fcd
https://hg.mozilla.org/integration/mozilla-inbound/rev/706147f947c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/33b34208ab03
https://hg.mozilla.org/integration/mozilla-inbound/rev/01b94e9f414e
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c72aa605302
https://hg.mozilla.org/integration/mozilla-inbound/rev/e4fd08e815a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b5ae79da708
https://hg.mozilla.org/integration/mozilla-inbound/rev/a26838a7eb48
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe15418497fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/123a397f5b7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9f7956706f4
https://hg.mozilla.org/integration/mozilla-inbound/rev/318b72381dcb
https://hg.mozilla.org/integration/mozilla-inbound/rev/10e77d478216
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f5d781e2fb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ae343665528
https://hg.mozilla.org/integration/mozilla-inbound/rev/227989411ad6

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/05254ae81fcd
https://hg.mozilla.org/mozilla-central/rev/706147f947c3
https://hg.mozilla.org/mozilla-central/rev/33b34208ab03
https://hg.mozilla.org/mozilla-central/rev/01b94e9f414e
https://hg.mozilla.org/mozilla-central/rev/7c72aa605302
https://hg.mozilla.org/mozilla-central/rev/e4fd08e815a2
https://hg.mozilla.org/mozilla-central/rev/7b5ae79da708
https://hg.mozilla.org/mozilla-central/rev/a26838a7eb48
https://hg.mozilla.org/mozilla-central/rev/fe15418497fc
https://hg.mozilla.org/mozilla-central/rev/123a397f5b7b
https://hg.mozilla.org/mozilla-central/rev/a9f7956706f4
https://hg.mozilla.org/mozilla-central/rev/318b72381dcb
https://hg.mozilla.org/mozilla-central/rev/10e77d478216
https://hg.mozilla.org/mozilla-central/rev/5f5d781e2fb1
https://hg.mozilla.org/mozilla-central/rev/6ae343665528
https://hg.mozilla.org/mozilla-central/rev/227989411ad6