844493 - MediaPipeline::Init() uses NS_DISPATCH_SYNC from the mainthread to STS

Status: RESOLVED FIXED

(Core :: WebRTC: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

Attachment: callstack

This causes critical problems such as JS reentrancy.

There are other instances; this is just one

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Attachment: trap out bad uses of NS_DISPATCH_SYNC in RUN_ON_THREAD

Comment 2

[Randell Jesup [:jesup] (needinfo me)]

Attachment: trap out bad uses of NS_DISPATCH_SYNC in RUN_ON_THREAD

Comment 3

[Eric Rescorla (:ekr)]

1. Since we currently do exactly this, landing this patch will just cause crashes.
2. Why are you landing this in RUN_ON_THREAD? If this is a system invariant, then it
should be in nsThread::Dispatch().

Comment 4

[Eric Rescorla (:ekr)]

And nsSocketTransportService::Dispatch(), etc.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

I'm not landing the patch; that was for diagnosing where we might be breaking this invariant.  It was to help explain the callstack (not that it was surprising at all).

This can cause Really Bad Things to happen.  Jan-ivar's issues are just the tip of the iceberg.  And like I said, it can cause weird re-entrancies.

I probably will file a bug for putting this in more general Dispatch routines, but that's a longer-term (or middle-term) thing.  This was easy and would entirely focus on our stuff (though it will miss some).

Comment 6

[Eric Rescorla (:ekr)]

What exactly do you believe the problem is with doing a SYNC dispatch from the main thread from
an event which is at the top of the event loop? Other than potentially stalling the main thread
while we wait for service, that is.

Comment 7

[Eric Rescorla (:ekr)]

The reason I ask about c6 is that it helps define the relevant urgency of the bug.

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Because SYNC dispatches spin the event loop - in this case the MainThread event loop, which can cause JS to become reentrant.  It's really only safe from a thread where you know it's safe; typically one where you control all the events occurring and know if reordering of the event queue and reentrancy on the same thread won't break any assumptions (like code that assumes "I'm on the XXX thread and so are all other users, I'll assume I don't need to lock access to this structure".

JIB was seeing the JS engine reenter JS even though it hadn't reached a 'stable state'.  That was what keyed me off to go check.

[12:56]	khuey	are you using DISPATCH_SYNC on the main thread?
[12:57]	khuey	as in, the main thread is sync dispatching to another thread?
[12:57]	khuey	if so, r-

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

See Bug 842749, Comment 5 for the JS re-entrancy I was seeing.

Comment 10

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

https://developer.mozilla.org/en-US/docs/Code_snippets/Threads#Waiting_for_a_background_task_to_complete

Comment 11

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Comment 12

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Comment 13

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Comment 14

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs

Comment 15

[Eric Rescorla (:ekr)]

https://tbpl.mozilla.org/?tree=Try&rev=95f02e4036d4

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 720750 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs

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

This has been tested locally. Waiting on try.

Comment 17

[Timothy B. Terriberry (:derf)]

Comment on attachment 720750 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs

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

I haven't finished reviewing the whole patch, but I'll go ahead and give this an r- for the PushLayers issue.

::: media/mtransport/nricectx.cpp
@@ +524,3 @@
>    state_ = state;
> +
> +  switch(state_) {

Could you move the ICE_CTX_GATHERED signal here, too? It is confusing to have some of the state transitions signaled from SetState(), but not all of them. AFAICT, this just means moving the SetState() call for it after EmitAllCandidates().

::: media/mtransport/transportflow.cpp
@@ +43,1 @@
>  nsresult TransportFlow::PushLayers(std::queue<TransportLayer *> layers) {

Not to juberti you further, but a std::queue is a pretty heavyweight object (it contains a std::deque, see bug 830100 comment 7) and you're copying it by value multiple times.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +2693,5 @@
>        return NULL;
>      }
> +
> +    // Note: the flow may not actually have any layers,
> +    // because the push might have somehow failed.

The past tense here is confusing... the push may not even have executed yet.

@@ +2698,1 @@
>      pc->media()->AddTransportFlow(level, rtcp, flow);

So, this is potentially unsafe, as we can expose the flow to getting signal handlers registered on it before the layers are all pushed (e.g., in PeerConnectionImpl::ConnectDataConnection()). Because PushLayer() hooks up signal handlers and calls Inserted() on each layer as it is pushed, which can in turn fire such signals, they might get delivered to the top level before all the layers are pushed, which is Bad(TM).

Comment 18

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 19

[Eric Rescorla (:ekr)]

Comment on attachment 721396 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

I have seen one crash due to linked_ptr. I do not know if it is from this change or not. I have been unable to repro.

Comment 20

[Timothy B. Terriberry (:derf)]

Comment on attachment 721396 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

Getting there, but a few too many things still to fix for me to be comfortable giving an r+.

::: media/mtransport/nricectx.cpp
@@ +522,5 @@
>  
>  
>  void NrIceCtx::initialized_cb(NR_SOCKET s, int h, void *arg) {
>    NrIceCtx *ctx = static_cast<NrIceCtx *>(arg);
> +  

WS.

::: media/mtransport/transportflow.cpp
@@ +56,5 @@
> +  while (!layers->empty()) {
> +    TransportLayer *old_layer = layers_.empty() ? nullptr : layers_.front();
> +    layer = layers->front();
> +
> +    nsresult rv = layer->Init();

This is shadowing the outer rv. If Init() fails, you won't clean up the push, because the outer rv is still NS_OK.

@@ +73,5 @@
> +    while (!layers->empty()) {
> +      delete layers->front();
> +      layers->pop();
> +    }
> +    

WS.

@@ +75,5 @@
> +      layers->pop();
> +    }
> +    
> +    // Now destroy the rest of the flow, because it's no longer
> +    // in an acceptable state.

AFAICT, you're not removing the layer pointers from the deque, which means the destructor will try to delete them again (not to mention anyone else trying too access them without checking the error status will be sad).

@@ +80,5 @@
> +    for (std::deque<TransportLayer *>::iterator it = layers_.begin();
> +         it != layers_.end(); ++it) {
> +      delete *it;
> +    }
> +    

WS.

@@ +91,5 @@
> +
> +  // Finally, attach ourselves to the top layer.
> +  layer->SignalStateChange.connect(this, &TransportFlow::StateChange);
> +  layer->SignalPacketReceived.connect(this, &TransportFlow::PacketReceived);
> +  

WS.

::: media/mtransport/transportflow.h
@@ +40,5 @@
>    nsresult PushLayer(TransportLayer *layer);
>  
>    // Convenience function to push multiple layers on. Layers
>    // are pushed on in the order that they are in the queue.
> +  // Any failures cause the flow to become inoperable and

Since we don't have our own state variable, failure isn't actually sticky (i.e., calling state() after the SignalStateChanged(this, TransportLayer::TS_ERROR) signal above will return TS_NONE). This makes it impossible to prevent further operations on this flow. That should be documented at the very least. "Inoperable" here is misleading... another call to PushLayers() will succeed.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +78,5 @@
>      if (NS_FAILED(res)) {
>        MOZ_MTLOG(PR_LOG_ERROR, "Error calling TransportReady()");
>        return res;
>      }
>    } else {

So, if PushLayers() manages to fail before you get here (which it will, since it was dispatched to the STS thread before Init_s()), state() will return TS_NONE and you'll never get a signal. That means that TransportFailed_s() never gets called, and we'll never notice that this flow is never going to open.

@@ +123,5 @@
>    bool rtcp = !(flow == rtp_transport_);
>    State *state = rtcp ? &rtcp_state_ : &rtp_state_;
>  
> +  // TODO([e-mail]): implement some kind of notification on
> +  // failure.

TransportFailed() already had one of these TODOs. Can I get a bug number here? This is obviously becoming more important as we make more failures asynchronous.

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.h
@@ +100,5 @@
>      MOZ_ASSERT(!stream_);  // Check that we have shut down already.
>    }
>  
> +
> +  // Must be called on the main thread.

Shouldn't it be ShutdownTransport_m() then? In general MediaPipeline does not seem very consistent about which methods get thread suffixes.

@@ +101,5 @@
>    }
>  
> +
> +  // Must be called on the main thread.
> +  // Must be called before shutdown media.

s/shutdown media/ShutdownMedia()/ please.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +126,5 @@
>      mIceStreams[i]->SignalReady.connect(this, &PeerConnectionMedia::IceStreamReady);
>    }
>  
> +  // TODO([e-mail]): When we have a generic error reporting mechanism,
> +  // figure out how to report that StartGathering failed.

See above about a bug #.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +181,5 @@
>    void ExpectVideo(const mozilla::TrackID);
>    unsigned AudioTrackCount();
>    unsigned VideoTrackCount();
>  
> +  void DetachTransport() {

DetachTransport_s()?

@@ +183,5 @@
>    unsigned VideoTrackCount();
>  
> +  void DetachTransport() {
> +    // walk through all the MediaPipelines and call the shutdown
> +    // functions for transport. Must be on the STS thread.

Assert please.

@@ +192,4 @@
>      }
> +  }
> +
> +  void DetachMedia() {

DetachMedia_m()?

@@ +193,5 @@
> +  }
> +
> +  void DetachMedia() {
> +    // walk through all the MediaPipelines and call the shutdown
> +    // functions. Must be on the main thread.

Assert please.

@@ +225,5 @@
>    }
>    void StorePipeline(int aTrack, bool aIsVideo,
>                       mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> +  void DetachTransport() {

DetachTransport_s()?

@@ +227,5 @@
>                       mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> +  void DetachTransport() {
> +    // walk through all the MediaPipelines and call the shutdown
> +    // transport functions. Must be on the STS thread.

Assert please.

@@ -217,5 @@
>                       mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> -  void Detach() {
> -    // walk through all the MediaPipelines and disconnect them.
> -    // XXX we should clear the mTypes map

We lost the XXX comment here. I assume it still applies.

@@ +236,4 @@
>      }
> +  }
> +
> +  void DetachMedia() {

DetachMedia_m()?

@@ +237,5 @@
> +  }
> +
> +  void DetachMedia() {
> +    // walk through all the MediaPipelines and call the shutdown
> +    // media functions. Must be on the main thread.

Assert please.

Comment 21

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 22

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 23

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 24

[Timothy B. Terriberry (:derf)]

Comment on attachment 727342 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

r=me with nits fixed.

::: media/mtransport/nricectx.cpp
@@ +559,5 @@
> +    case ICE_CTX_FAILED:
> +      SignalFailed(this);
> +      break;
> +    default:
> +      break;

Should we assert here?

::: media/mtransport/transportflow.cpp
@@ +50,2 @@
>  
> +  // Don't allow pushes once we are in error state.

Don't we need this in PushLayer, too?

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +392,5 @@
>  
>    (void)conduit_->ReceivedRTPPacket(inner_data, out_len);  // Ignore error codes
>  }
>  
> +

Spurious whitespace change.

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +183,5 @@
>    void ExpectAudio(const mozilla::TrackID);
>    void ExpectVideo(const mozilla::TrackID);
>    unsigned AudioTrackCount();
>    unsigned VideoTrackCount();
> +  void DetachTransport_s(); 

WS.

Comment 25

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 26

[Eric Rescorla (:ekr)]

Comment on attachment 728675 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

carried r+ forward from derf

Comment 27

[Eric Rescorla (:ekr)]

Comment on attachment 728675 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

carried r+ forward from derf

Comment 28

[Eric Rescorla (:ekr)]

https://tbpl.mozilla.org/?tree=Try&rev=7e913bc3bb45

Comment 29

[Adam Roach [:abr]]

Comment on attachment 727342 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

Generally, looks good. I'm only r- for now because (1) I'd like to have a look at things after fixing the ownership contract for PushLayers, and (2) I want to follow up on the SourceStreamInfo class hierarchy thing.

::: media/mtransport/nricectx.cpp
@@ +411,5 @@
>  
>  nsresult NrIceCtx::StartGathering() {
> +  MOZ_ASSERT(ctx_->state == ICE_CTX_INIT);
> +  if (ctx_->state != ICE_CTX_INIT)
> +    return NS_ERROR_FAILURE;

Since we can't check the response code in an async call, please add logging to this failure condition.

::: media/mtransport/nricectx.h
@@ +198,5 @@
>  
>    // Signals to indicate events. API users can (and should)
>    // register for these.
> +  // TODO([e-mail]): refactor this to be state change instead
> +  // so we don't need to keep adding signals?

Add a bug number?

::: media/mtransport/transportflow.cpp
@@ +44,5 @@
> +// This is all-or-nothing.
> +nsresult TransportFlow::PushLayers(std::queue<TransportLayer *>* layers) {
> +  MOZ_ASSERT(!layers->empty());
> +  if (layers->empty()) {
> +    return NS_ERROR_INVALID_ARG;

Now that this method is called async, we've lost the ability to communicate these errors. While that's somewhat problematic in itself, I would think that we need, at a minimum, to add error-level logging here so we can untangle problems if they arise. Ditto for all other non-success returns in this function.

::: media/mtransport/transportflow.h
@@ +51,2 @@
>    // TODO([e-mail]): Change layers to be ref-counted.
> +  nsresult PushLayers(std::queue<TransportLayer *>* layers);

This signature change is problematic from the perspective of the implied ownership contract for the layers queue. See my comment in VcmSIPCCBinding.cpp.

::: media/webrtc/signaling/src/media/VcmSIPCCBinding.cpp
@@ +769,5 @@
>      return VCM_ERROR;
>    }
>    nsresult rv = pc.impl()->media()->ice_ctx()->thread()->Dispatch(
> +    WrapRunnable(pc.impl()->media()->ice_ctx(), &NrIceCtx::StartChecks),
> +      NS_DISPATCH_NORMAL);

We need a todo / bug # for StartChecks failure here.

@@ +2686,5 @@
>  
>      // Layers are now owned by the flow.
>      nsresult rv = pc->media()->ice_ctx()->thread()->Dispatch(
> +        WrapRunnable(flow, &TransportFlow::PushLayers, layers),
> +        NS_DISPATCH_NORMAL);

While the is safe, it's a bit too clever. The (new) signature for PushLayers is:

nsresult PushLayers(std::queue<TransportLayer *>* layers);

So, when you perform this dispatch, the runnable takes ownership of the layers. When it is executed, the runnable creates a raw-pointer alias to layers. The nsAutoPtr does keep things alive long enough for PushLayers to complete, but it requires a lot of rumination to figure out that this all works fine (since the safety only arises as a side effect of how your templatized runnables are implemented).

In my experience, there is very little call for passing pointers as parameters in C++; it makes the ownership contract ambiguous. If the caller holds on to the object, a reference makes that fact plain; similarly, if the caller surrenders ownership, an auto_ptr (or similar) serves to make that agreement clear. In this case, since PushLayers is basically destroying the queue, I think you want it to take ownership (i.e., to change the PushLayers signature to take an nsAutoPtr as its argument).

@@ +2693,5 @@
>        return NULL;
>      }
> +
> +    // Note: the flow may not actually have any layers,
> +    // because the push might have somehow failed.

I think we need a TODO and a bug number here. The inability to communicate error conditions in the setup of the transport pipeline seems that it will frustrate attempts to return sensible error messages to the javascript (in the spirit of bug 834270).

::: media/webrtc/signaling/src/mediapipeline/MediaPipeline.cpp
@@ +57,2 @@
>    RUN_ON_THREAD(sts_thread_,
> +		WrapRunnable(

Tab

@@ +57,5 @@
>    RUN_ON_THREAD(sts_thread_,
> +		WrapRunnable(
> +                    nsRefPtr<MediaPipeline>(this),
> +                    &MediaPipeline::Init_s),
> +		NS_DISPATCH_NORMAL);

Tab

@@ +78,2 @@
>      if (NS_FAILED(res)) {
>        MOZ_MTLOG(PR_LOG_ERROR, "Error calling TransportReady()");

Now that we're dispatching sync, this is the end of the line for error codes. I would suggest including the result code in the log message. Also, please update the log message function name to match the name of the function that failed.

@@ +95,5 @@
>            return res;
> +	}
> +      } else if (rtcp_transport_->state() == TransportLayer::TS_ERROR) {
> +        MOZ_MTLOG(PR_LOG_ERROR, "RTCP transport is already in error state");
> +	TransportFailed_s(rtcp_transport_);

Tab

@@ +96,5 @@
> +	}
> +      } else if (rtcp_transport_->state() == TransportLayer::TS_ERROR) {
> +        MOZ_MTLOG(PR_LOG_ERROR, "RTCP transport is already in error state");
> +	TransportFailed_s(rtcp_transport_);
> +	return NS_ERROR_FAILURE;

Tab

::: media/webrtc/signaling/src/peerconnection/PeerConnectionImpl.cpp
@@ +1310,5 @@
>    nsRefPtr<PeerConnectionImpl> pc(this);
>    RUN_ON_THREAD(mThread,
>                  WrapRunnable(pc,
> +                             &PeerConnectionImpl::IceStateChange_m,
> +			     kIceWaiting),

Tab

@@ +1323,5 @@
>    nsRefPtr<PeerConnectionImpl> pc(this);
>    RUN_ON_THREAD(mThread,
>                  WrapRunnable(pc,
> +                             &PeerConnectionImpl::IceStateChange_m,
> +			     kIceConnected),

Tab

@@ +1336,5 @@
> +  nsRefPtr<PeerConnectionImpl> pc(this);
> +  RUN_ON_THREAD(mThread,
> +                WrapRunnable(pc,
> +                             &PeerConnectionImpl::IceStateChange_m,
> +			     kIceFailed),

Tab

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.cpp
@@ +106,5 @@
> +           mPipelines.begin(); it != mPipelines.end();
> +       ++it) {
> +    it->second->ShutdownMedia_m();
> +  }
> +}

This kind of code duplication is kind of a red flag. Wouldn't it make more sense to refactor RemoteSourceStreamInfo and LocalSourceStreamInfo to derive from a common SourceStreamInfo class and then implement each of these once?

@@ +148,5 @@
>                                              &PeerConnectionMedia::IceGatheringCompleted);
>    mIceCtx->SignalCompleted.connect(this,
>                                     &PeerConnectionMedia::IceCompleted);
> +  mIceCtx->SignalFailed.connect(this,
> +				&PeerConnectionMedia::IceFailed);

Tab

::: media/webrtc/signaling/src/peerconnection/PeerConnectionMedia.h
@@ +183,5 @@
>    void ExpectAudio(const mozilla::TrackID);
>    void ExpectVideo(const mozilla::TrackID);
>    unsigned AudioTrackCount();
>    unsigned VideoTrackCount();
> +  void DetachTransport_s(); 

ws

@@ -218,5 @@
>                       mozilla::RefPtr<mozilla::MediaPipeline> aPipeline);
>  
> -  void Detach() {
> -    // walk through all the MediaPipelines and disconnect them.
> -    // XXX we should clear the mTypes map

Does this XXX go away with the current code? I don't see where we're taking care of it. Perhaps this XXX should be copied into the new implementation?

::: media/webrtc/signaling/test/mediapipeline_unittest.cpp
@@ +79,5 @@
>                           TransportLayerDtls::SERVER);
>      audio_flow_->PushLayer(audio_dtls_);
>    }
>  
> +  virtual void CreatePipelines() = 0;

Following the naming convention established for methods that must be called on a specific thread, shouldn't this be CreatePipelines_s()?

::: media/webrtc/signaling/test/signaling_unittests.cpp
@@ +564,5 @@
>  
>      EXPECT_TRUE_WAIT(sipcc_state() == sipcc::PeerConnectionImpl::kStarted,
>                       kDefaultTimeout);
> +    EXPECT_TRUE_WAIT(ice_state() == sipcc::PeerConnectionImpl::kIceWaiting ||
> +		     ice_state() == sipcc::PeerConnectionImpl::kIceFailed, 5000);

Tab

Comment 30

[Adam Roach [:abr]]

Comment on attachment 728675 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

Fixing my r flag

Comment 31

[Eric Rescorla (:ekr)]

Attachment: Remove NS_DISPATCH_SYNCs; WIP

Remove NS_DISPATCH_SYNCs

Comment 32

[Eric Rescorla (:ekr)]

Comment on attachment 729190 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

carrying forward r+ from derf.

Comment 33

[Adam Roach [:abr]]

Comment on attachment 729190 [details] [diff] [review]
Remove NS_DISPATCH_SYNCs; WIP

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

Looks good to me.

Comment 34

[Eric Rescorla (:ekr)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/da67bb9e1691

Comment 35

[Ryan VanderMeulen [:RyanVM]]

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

Comment 36

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Eric, do we have to backport the patch to Aurora (21.0)?

Comment 37

[Jason Smith [:jsmith]]

(In reply to Henrik Skupin (:whimboo) from comment #36)
> Eric, do we have to backport the patch to Aurora (21.0)?

Not at this point. We just prefed off on FF 21 per a different bug. No point of doing uplifts at this point to FF 21.