1199295 - Investigate loadingPrincipal for loadInfo in uriloader/prefetch

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

AS smaug points out [1] we should consider passing a different loadingPrincipal in uriloader/prefetch. Maybe we can find a better principal than the systemPrincipal for those cases [2]. As of know it's totally fine to use the systemPrincipal, because these callsites didn't perform any security checks anyway, but maybe they should. Long story short, using systemPrincipal does not make things worse, but maybe we could improve security here.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1192948#c5
[2] http://hg.mozilla.org/mozilla-central/diff/d4ced32fc77b/uriloader/prefetch/nsOfflineCacheUpdate.cpp

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can someone explain what uriloader/prefetch does? I.e. what feature of Firefox is it used for?

Comment 2

[Tanvi Vyas[:tanvi]]

(In reply to Jonas Sicking (:sicking) from comment #1)
> Can someone explain what uriloader/prefetch does? I.e. what feature of
> Firefox is it used for?

Perhaps Honza or smaug can answer this.

Comment 3

[Honza Bambas (:mayhemer)]

uriloader/prefetch does two things:
- offline application cache update processing (nsOfflineCacheUpdate and Service), something we still maintain
- prefetch service (used from nsContentSink, and derivatives) for preloading (precaching) hrefs and other stuff (how exactly this works and whether it's actually used I really don't know and I'm no expert to that exact code)

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

For both of those we need to get a correct principal as well as correct security flags and nsContentPolicyType.

So I think we need to require all callers to pass that information in. Fortunately it should also mean that we can remove security checks at those callsites.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #4)
> For both of those we need to get a correct principal as well as correct
> security flags and nsContentPolicyType.
> 
> So I think we need to require all callers to pass that information in.
> Fortunately it should also mean that we can remove security checks at those
> callsites.

Alrighty, let's have a look what we can do here.

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_uriloader_prefetch.patch

Jonas, here is a first attempt of replacing mDocumentURI with mLoadingPrincipal and hence passing the loadingPrincipal all the way down to uriloader/prefetch where we actually create the channel.

What do you think about that approach? Before doing any more work here I would like you to confirm this is an approach we are considering and if we also need to pass down other information, like e.g. a contentType.

Comment 7

[Honza Bambas (:mayhemer)]

Comment on attachment 8658352 [details] [diff] [review]
bug_1199295_uriloader_prefetch.patch

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

::: dom/offline/nsDOMOfflineResourceList.cpp
@@ +454,5 @@
>  {
>    nsresult rv = Init();
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  // i think we can remove that check

why?

::: uriloader/prefetch/nsIOfflineCacheUpdate.idl
@@ +194,5 @@
>     */
>    readonly attribute uint64_t byteProgress;
>  };
>  
> +[scriptable, uuid(a297a334-bcae-4779-a564-555593edc96b)]

note that I'm modifying this code as well right now in bug 1165256

@@ +271,5 @@
>       *        The pref branch to use to check the
>       *        offline-apps.allow_by_default pref.  If not specified,
>       *        the pref service will be used.
>       */
> +    // maybe remove?

why should these be removed?

Comment 8

[Honza Bambas (:mayhemer)]

Jonas, what is the time frame for this bug?  I would like to base a patch for one of the NSec bugs on patches proposed here.  Also to avoid conflicts and save some work (bug 1165256 may use rewrite anyway after this lands).  

There is no critical need tho, I already have a patch doing what I want for that bug.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8658352 [details] [diff] [review]
bug_1199295_uriloader_prefetch.patch

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

My understanding is that the spec requires the manifest to be same-origin with the document that links to the manifest. Is that correct?

::: dom/base/nsContentSink.cpp
@@ +1120,1 @@
>      rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, true, false);

If this is just there to ensure that we enforce that the manifest is same-origin with the document that links to it, then yeah, I think we can remove this if you make the other changes below.

@@ +1123,5 @@
>      }
>      else {
>        // Only continue if the document has permission to use offline APIs or
>        // when preferences indicate to permit it automatically.
> +      // not sure if we can remove that check

I don't think we can remove these. It looks to me like this is enforcing things other than origin policies.

::: dom/base/nsGlobalWindow.cpp
@@ +4424,5 @@
>  
>      nsCOMPtr<nsIURI> manifestURI;
>      nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI));
>  
> +    // uri is different from mDoc->NodePrincipal()

Different how? And when?

::: uriloader/prefetch/nsIOfflineCacheUpdate.idl
@@ +271,5 @@
>       *        The pref branch to use to check the
>       *        offline-apps.allow_by_default pref.  If not specified,
>       *        the pref service will be used.
>       */
> +    // maybe remove?

I don't think these can be removed. These enforce different policies than network policies.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +181,1 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

This should use SEC_REQUIRE_SAME_ORIGIN_DATA_IS_BLOCKED I think, no?

@@ +181,2 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,
>                         nsIContentPolicy::TYPE_OTHER,

Technically we should have a separate type for appcache manifests I think, but I suspect that is not worth adding since we are deprecating this code anyway.

@@ +374,1 @@
>                         nsILoadInfo::SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL,

Is this for loading the resources pointed to by the manifest? If so using SEC_ALLOW_CROSS_ORIGIN_DATA_IS_NULL is probably correct.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #8)
> Jonas, what is the time frame for this bug?  I would like to base a patch
> for one of the NSec bugs on patches proposed here.  Also to avoid conflicts
> and save some work (bug 1165256 may use rewrite anyway after this lands).  
> 
> There is no critical need tho, I already have a patch doing what I want for
> that bug.

I'll let you and Christoph decide this since the two of you are the ones writing the code. I'll try to stay on top of feedback requests, though these changes are complex enough that someone else should probably review.

Comment 11

[Honza Bambas (:mayhemer)]

Christoph, are you OK with making this bug block a priority bug for which deadline is Oct 02?

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #11)
> Christoph, are you OK with making this bug block a priority bug for which
> deadline is Oct 02?

I am fine with moving this bug to the front of my priority queue. Let's hope that we get review requests in quite quickly.

Jonas, from your feedback I suspect you are fine with just passing the principal all the way do the point where we create the new channel, but just to make sure, nothing else besides the principal, right?

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We need to have access to all the normal information. So if we at the callsite know what we are loading, and thus can hardcode things like security flags and contentpolicytype, then simply passing down the principal sounds fine.

But in comment 3 Honza mentioned that this code is also used for prefetching resources that we detect during HTML parsing (at least that was my understanding of comment 3), then I would expect that we need to use different policies for different types of resources that are being prefetched.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #13)
> We need to have access to all the normal information. So if we at the
> callsite know what we are loading, and thus can hardcode things like
> security flags and contentpolicytype, then simply passing down the principal
> sounds fine.
> 
> But in comment 3 Honza mentioned that this code is also used for prefetching
> resources that we detect during HTML parsing (at least that was my
> understanding of comment 3), then I would expect that we need to use
> different policies for different types of resources that are being
> prefetched.

If that is the case, then it would make sense to pass an 'nsILoadinfo*' as an argument rather than an nsIPrincipal. Honza, what do you think?

Comment 15

[Honza Bambas (:mayhemer)]

I'm OK with both.  What is the time frame for a first patch I could start building patch for my bug 1165256 on?

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It's hard for me to tell if a LoadInfo is the right thing to use for the prefetch code. Where is that code?

The current patch looks fine, no need to use a LoadInfo there, right?

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> It's hard for me to tell if a LoadInfo is the right thing to use for the
> prefetch code. Where is that code?

I honestly don't think that any of this code is used for prefetching resources by the HTML parser (if at all we interpret comment 3 correctly and that's what Honza is thinking). Wouldn't any of my changes cause compile errors if that is the case?

> The current patch looks fine, no need to use a LoadInfo there, right?

So far no need for passing a LoadInfo, I agree. I think we are fine just passing down the principal. Who would be a good reviewer for this patch?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_uriloader_prefetch.patch

I agree with the feedback comments from Honza (Comment 7) and Jonas (Comment 9) and we can't remove those security checks from the *.idl and also not from the code, because we don't end up creating a new channel in all of those cases.

I think what we should do is only pass the loadingPrincipal instead of the uri all the way down. So we end up having the correct loadingPrincipal on the channel and leave everything else untouched.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Note first comment in comment 9. I think one of the checks can be removed.

Comment 20

[Honza Bambas (:mayhemer)]

According a reviewer - definitely the offline cache changes should go through me.  The other code depends.  Since I really don't know how it works, probably someone else should do that.  No idea who, tho.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

Alright, lets give this a first spin, just replacing the uri with the principal.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_dom_changes.patch

(In reply to Jonas Sicking (:sicking) from comment #9)
> My understanding is that the spec requires the manifest to be same-origin
> with the document that links to the manifest. Is that correct?
> 
> ::: dom/base/nsContentSink.cpp
> @@ +1120,1 @@
> >      rv = mDocument->NodePrincipal()->CheckMayLoad(manifestURI, true, false);
> 
> If this is just there to ensure that we enforce that the manifest is
> same-origin with the document that links to it, then yeah, I think we can
> remove this if you make the other changes below.

I think that is the case, but smaug probably knows better if it's safe to remove CheckMayLoad there.

Comment 23

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

Comment on attachment 8663236 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch

So doesn't this change the behavior, because we move the sameOrigin check to happen later.
If the manifest isn't same origin, we don't call SelectDocAppCacheNoManifest anymore. I wouldn't change the behavior of this fragile, deprecated code.
Or explain why this patch wouldn't change the behavior.

In other words, I'm not familiar enough with this code to be ready to say that such behavioral changes are ok.

The security check was added in https://bugzilla.mozilla.org/show_bug.cgi?id=402272 in GetSecurityManager()->CheckSameOriginURI form.

And if the check failed, calling SelectDocAppCacheNoManifest was added in
https://bugzilla.mozilla.org/show_bug.cgi?id=460353
Based on the assignee of the latter bug, mayhemer clearly might have some opinion on that.

Comment 24

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

Is there any harm to leave that mDocument->NodePrincipal()->CheckMayLoad there?

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Olli Pettay [:smaug] from comment #23)
> If the manifest isn't same origin, we don't call SelectDocAppCacheNoManifest
> anymore. I wouldn't change the behavior of this fragile, deprecated code.

I don't think there is any harm in leaving the additional same origin check there. The only downside is that SOP is (potentially) enforced twice, once on the callsite [mDocument->NodePrincipal()->CheckMayLoad] and once within AsyncOpen2().

As you mentioned, it's fragile code, and also deprecated code. I would rather not update those fragile bits and leave the [mDocument->NodePrincipal()->CheckMayLoad] instead of debugging all the cases where we potentially do not need the checkMayLoad case on the callsite. Let's just wait till it's completely deprecated - can we all live with that?

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Keeping the CheckMayLoad does mean that we end up with different fallback behavior for different form of network policy violations. For example it means that having the original URL be cross-origin is treated differently from for example:

* A redirect to the same cross-origin URL.
* A URL which violates CSP.
* A URL which violates some security checks done by an addon installed by the user.

So generally speaking it's better to let the channel do all error checking and then call fallback handling if the channel indicates an error.

That said, I agree that since this code is about to be deprecated, I think trying to fix it up to handle the above scenarios perfectly is no worth it. So just keeping the CheckMayLoad call seems fine with me.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_dom_changes.patch

(In reply to Jonas Sicking (:sicking) from comment #26)
> That said, I agree that since this code is about to be deprecated, I think
> trying to fix it up to handle the above scenarios perfectly is no worth it.
> So just keeping the CheckMayLoad call seems fine with me.

I totally agree with that in general we should perform security checks only in one place, but since this code is about to be deprecated I am all in favor of not going trhough the trouble of fixing up that fragile piece of code. Flagging smaug again for review with the security check kept in place.

Comment 28

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

Comment on attachment 8663776 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch


>-      updateService->ScheduleOnDocumentStop(manifestURI, mDocumentURI, domdoc);
>+      updateService->ScheduleOnDocumentStop(manifestURI, mDocument->NodePrincipal(), domdoc);
So we do use document uri somewhere under ScheduleOnDocumentStop, so better to verify this handling is right.
Note, iframe which loads data: has its parent's principal, but data: as documentURI, so using parent's principal's uri
as the document uri for the iframe would be rather big change, and I can't see how that could be right.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_dom_changes.patch

Thanks smaug for the feedback and I think you are right. In that case we just extend the argument list to pass the loadingPrincipal all the way down to the point where we create the channel.

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

Honza, we are going to keep this real simple and just pass the principal all the way down instead of any other modifications.

Comment 31

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

Comment on attachment 8664035 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_dom_changes.patch

>@@ -4432,17 +4432,17 @@ nsGlobalWindow::GetApplicationCache(Erro
>     if (aError.Failed()) {
>       return nullptr;
>     }
> 
>     nsCOMPtr<nsIURI> manifestURI;
>     nsContentUtils::GetOfflineAppManifest(mDoc, getter_AddRefs(manifestURI));
> 
>     nsRefPtr<nsDOMOfflineResourceList> applicationCache =
>-      new nsDOMOfflineResourceList(manifestURI, uri, this);
>+      new nsDOMOfflineResourceList(manifestURI, uri, mDoc->NodePrincipal(), this);
Technically nothing guarantees mDoc isn't null here. So, could you add a 
if (!mDoc) {
  aError.Throw(NS_ERROR_FAILURE);
  return nullptr;
}
somewhere here. Or perhaps just add || !mDoc to
if (!webNav) {


>     virtual POfflineCacheUpdateChild* AllocPOfflineCacheUpdateChild(
>             const URIParams& manifestURI,
>             const URIParams& documentURI,
>+            const PrincipalInfo& loadingPrincipalInfo,
>             const bool& stickDocument,
>             const TabId& aTabId) override;
Mumbling about this code not using Mozilla coding style. I wouldn't might if you used aFoo for the new param, given the already
inconsistency

Comment 32

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #30)
> Created attachment 8664037 [details] [diff] [review]
> bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch
> 
> Honza, we are going to keep this real simple and just pass the principal all
> the way down instead of any other modifications.

Fantastic.  I can work with that really well, since it's easy to get OriginAttributes from nsIPrincipal.  Thanks!

I'll take a look at the patch soon.

Comment 33

[Honza Bambas (:mayhemer)]

Comment on attachment 8664037 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

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

::: uriloader/prefetch/OfflineCacheUpdateChild.cpp
@@ +440,5 @@
>      // Need to addref ourself here, because the IPC stack doesn't hold
>      // a reference to us. Will be released in RecvFinish() that identifies 
>      // the work has been done.
>      ContentChild::GetSingleton()->SendPOfflineCacheUpdateConstructor(
> +        this, manifestURI, documentURI, loadingPrincipalInfo,

the appropriate IPDL change should be in this patch, not in the dom parts.  but that is more about how badly the code is organized.

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +1249,5 @@
>          do_GetService(NS_APPLICATIONCACHESERVICE_CONTRACTID, &rv);
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      mDocumentURI = aDocumentURI;
> +    mLoadingPrincipal = aLoadingPrincipal;

redundant?

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

Carrying over r+ from Honza.

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #33)
> the appropriate IPDL change should be in this patch, not in the dom parts. 
> but that is more about how badly the code is organized.

Initially I split the patches based on directories. Since the *.ipdl is within dom/ that made sense, but I agree with you, hence I moved the *.ipdl into that patch.
 
> ::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
> @@ +1249,5 @@
> > +    mLoadingPrincipal = aLoadingPrincipal;
> 
> redundant?

Indeed, redundant, because we call initInternal right above. I think that initialization of mDocumentURI confused me. Anyway, removed the additional initialization of mLoadingPrincipal.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_dom_changes.patch

Carrying over r+ from smaug.

(In reply to Olli Pettay [:smaug] from comment #31)
> Technically nothing guarantees mDoc isn't null here. So, could you add a 

Thanks for catching that. I added the proposed null-check.

> Mumbling about this code not using Mozilla coding style. I wouldn't might if
> you used aFoo for the new param, given the already inconsistency

Yeah, I don't like that either, so I changed all the code I added to use the right codingStyle including the leading 'a' as a prefix for all the incoming arguments.

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Let's make sure everything is fine:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=541be9109253

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_uriloader_changes_2.patch

Honza, when running dom/apps/tests/test_app_update.html I realized we also have to propagate the loadingPrincipal for InitPartial(). Once r+ed, I will qfold that patch into the other one you already reviewed.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_callsites_dom_b2g_browser_mobile.patch

Jonas, do you feel comfortable reviewing those callsites? Probably 'dom/apps/Webapps.jsm' is the most tricky one, but we are already using the same technique of creating the principal within that file which gives me more confidence that this is the way to do it.

Comment 39

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_test_updates.patch

Honza, there are also quite so many netwerk tests, I suppose using the systemPrincipal is fine.

Comment 40

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8665782 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_callsites_dom_b2g_browser_mobile.patch

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

Yeah, I think we need the owners of the individual pieces of code here to review this code.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_callsites_b2g.patch

Comment 42

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_callsites_dom_apps.patch

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_callsites_browser.patch

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_callsites_mobile_android.patch

Comment 45

[:Gijs (he/him)]

Comment on attachment 8666136 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_callsites_browser.patch

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

rs=me

Comment 46

[Honza Bambas (:mayhemer)]

Comment on attachment 8665781 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_uriloader_changes_2.patch

Thanks.

Comment 47

[Christoph Kerschbaumer [:ckerschb]]

Honza, unfortunately bugzilla does not allow one to use more than one needinfo flag within a bug, hence you missed reviewing  bug_1199295_loadingprincipal_prefetch_test_updates.patch (see comment 39). Can you please have a look and then we have that landed in no time :-)

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 8665783 [details] [diff] [review]
bug_1199295_loadingprincipal_prefetch_test_updates.patch

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

::: dom/tests/mochitest/ajax/offline/test_updateCheck.html
@@ +34,4 @@
>  function manifestCached()
>  {
>    // Run first check for an update
> +  updateService.checkForUpdate(manifestURI, systemPrincipal, 0, false, {

you may need to use the window's principal here.  we do check the HTTP cache and this way you may tell the cache/appcache use a different jar (means that something may not be found when it should be).  this will work now, but may break later.

anyway, appcache is deprecated, so if this works, leave it...

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1199295_loadingprincipal_prefetch_uriloader_changes.patch

Just qfolded the two patches. Carrying over r+ from mayhemer.

Comment 50

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31da4a02d8b6
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9e1eb325c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d699f02b8d9e
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd3f94001ecd
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e9bf8fead19
https://hg.mozilla.org/integration/mozilla-inbound/rev/8d76980d5199
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb4a8da5c964

Comment 51

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/31da4a02d8b6
https://hg.mozilla.org/mozilla-central/rev/cf9e1eb325c8
https://hg.mozilla.org/mozilla-central/rev/d699f02b8d9e
https://hg.mozilla.org/mozilla-central/rev/dd3f94001ecd
https://hg.mozilla.org/mozilla-central/rev/5e9bf8fead19
https://hg.mozilla.org/mozilla-central/rev/8d76980d5199
https://hg.mozilla.org/mozilla-central/rev/eb4a8da5c964