1340163 - Origin-unique deviceId persistence (used in enumerateDevices and gUM constraints) is broken

Status: VERIFIED FIXED

(Core :: WebRTC: Audio/Video, defect, P1)

Description

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

STR:
1. Open https://jsfiddle.net/jib1/Lqo4paed/ and hit [Enumerate!] button
2. Open https://codepen.io/jib1/full/mRgwzx/ and hit [Enumerate!] button

Expected result: deviceIds are different for the two domains.

Actual result: deviceIds are the same for the two domains.

A regression from Bug 1320170.

The spec [1] mandates that deviceIds should be stable across all pages of the same origin, but unique to the origin. 

Note that generated deviceIds are saved to enumerate_devices.txt in the profile folder, and as of this regression, that stopped working, saving an entry with no origin associated. Just noting that, as this may have an effect on other releases.

[1] https://w3c.github.io/mediacapture-main/getusermedia.html#def-constraint-deviceId

Comment 1

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 2

[Andrea Marchesini [:baku]]

Attachment: part 2 - Origin in MediaParent

Comment 3

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 4

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8838477 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo

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

A good start, but I'm really nervous about adding optional fields on the identity type underpinning our entire security system.  I think we should require the existence of originNoSuffix if we are going to add it.  That way we can assert that the principal is properly formed, etc.

::: caps/BasePrincipal.cpp
@@ +320,5 @@
>  NS_IMETHODIMP
>  BasePrincipal::GetOriginNoSuffix(nsACString& aOrigin)
>  {
> +  if (mCachedOriginNoSuffix.IsEmpty()) {
> +    return GetOriginInternal(aOrigin);

I know its an existing issue, but I find it very confusing that GetOriginInternal() returns OriginNoSuffix.  Can you rename it to GetOriginNoSuffixInternal() in a separate patch before this?

@@ +681,5 @@
> +    return nullptr;
> +  }
> +
> +  principal->mCachedOriginNoSuffix = aOriginNoSuffix;
> +  return principal.forget();

I think originNoSuffix should probably be passed to nsPrincipal::Init() as a required value.  So the two variants of CreateCodePrincipal() should probably swap so the 2-arg version delegates to the 3-arg version.

::: caps/BasePrincipal.h
@@ +306,5 @@
>    OriginAttributes mOriginAttributes;
> +
> +  // This is the value passed as 3rd argument in CreateCodebasePrincipal. Mainly
> +  // it used by the IPC serialization of ContentPrincipalInfo.
> +  nsCString mCachedOriginNoSuffix;

I think this should always be set.  And it should include a comment describing when it can be different from the nsIURI mCodebase origin.

Also, shouldn't this be on nsPrincipal instead of BasePrincipal?

::: dom/workers/ServiceWorkerRegistrar.cpp
@@ +354,5 @@
>        GET_LINE(entry->scope());
>  
>        entry->principal() =
> +        mozilla::ipc::ContentPrincipalInfo(attrs, EmptyCString(),
> +                                           entry->scope());

So, the idea that the originNoSuffix is optional seems very confusing and error prone to me.  I think we should require originNoSuffix to be set always.

So, for future values we should start storing the originNoSuffix in the registrar file.

For back compat the safest thing to do would be to create a LegacyContentPrincipalInfo type without the originNoSuffix field.  ServiceWorkerRegistrar would be the only code that produces this type.  PrincipalInfoToPrincipal would then extract the originNoSuffix from the spec, create a new ContentPrincipalInfo, and perform the real serialization.

::: ipc/glue/BackgroundUtils.cpp
@@ +93,2 @@
>        rv = principal ? NS_OK : NS_ERROR_FAILURE;
>        if (NS_WARN_IF(NS_FAILED(rv))) {

While here, do you want to remove this bizarre conversion to nsresult for no reason?  It can just:

  if (NS_WARN_IF(!principal)) {
    return nullptr;
  }

@@ +221,5 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  
> +  nsAutoCString origin;

nit: It would be better to use nsCString here since we are passing them into ContentPrincipalInfo which also stores nsCString.  This would avoid an extra copy.  You could change the other nsAutoCString types in this method as well.

Comment 6

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

Are the Affected setting from :vladb correct?  especially as the bug mentioned as a source of the regression is marked as fixed on 53/54, and 52 not affected...

Comment 7

[Vlad Bacia-Mociran [:VladB]]

@Randell, I have verified this issue on all builds and the results were:

- latest Firefox v51.0.1 - issue can be reproduced
- latest Firefox Beta 52.0b6 - issue can be reproduced
- latest Firefox Dev 53.0a2 - issue cannot be reproduced
- latest Nightly 54.0a1 - issue cannot be reproduced

Please, can you explain why the affected versions were changed?

Comment 8

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

Vlad, how did you verify the issue? Note, in comment 0 the correct result is for the ids to be different.

If you were to open the same url (or same-origin urls) in different tabs, then the ids should be the same, not otherwise.

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 0 - renaming GetOriginInternal to GetOriginNoSuffixInternal

Comment 10

[Vlad Bacia-Mociran [:VladB]]

Jan-Ivar, you are right. I did another run and the IDs are different for Firefox v51.0.1 and latest Firefox Beta 52.0b6, and for latest Firefox Dev 53.0a2 and latest Nightly 54.0a1 the IDs are the same.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

I think we can clean up principal code a lot and I'm planning to do it in follow up bugs. But here, the first bits:

1. I moved Init() into BasePrincipal in order to force any principal type to set OriginAttributes and OriginNoSuffix.

2. Follow up to point 1 is that we don't need to have GetOriginNoSuffixInternal at all.

3. nsExpandedPrincipal::Create is introduced.

Comment 12

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

Comment on attachment 8839810 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo

>+BasePrincipal::CreateCodebasePrincipal(nsIURI* aURI,
>+                                       const OriginAttributes& aAttrs,
>+                                       const nsACString& aOriginNoSuffix)
> {
>+  MOZ_ASSERT(aURI);
This looks weird. Why we need aOriginNoSuffix if we already have aURI?

I so don't understand the setup at all :)
Could you explain?
Why ContentPrincipalInfo needs originNoSuffix when it already has spec?

When is it ok to not use spec? When is it ok to not use originNoSuffix?


Can't really review this patch without some more background information.
(or I need to review also what was done in bug 1320170 and I guess also bug 1319045)

Comment 13

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

I moved the generation of the origin in case it's EmptyString() in BasePrincipal.
And I also added a comment.

Comment 14

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

Comment on attachment 8842002 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo


>@@ -797,16 +843,19 @@ ServiceWorkerRegistrar::WriteData()
> 
>     buffer.Truncate();
>     buffer.Append(suffix.get());
>     buffer.Append('\n');
> 
>     buffer.Append(data[i].scope());
>     buffer.Append('\n');
> 
>+    buffer.Append(data[i].origin());
>+    buffer.Append('\n');
>+
I don't understand how .origin() is ever non-empty-string here. Because of this, r-
And if it is supposed to be originNoSuffix, perhaps better to call it such.


>@@ -592,17 +669,18 @@ TEST(ServiceWorkerRegistrar, TestDedupeW
>       reg.currentWorkerHandlesFetch() = true;
>       reg.cacheName() =
>         NS_ConvertUTF8toUTF16(nsPrintfCString("cacheName write %d", i));
>       reg.loadFlags() = nsIRequest::VALIDATE_ALWAYS;
> 
>       nsAutoCString spec;
>       spec.AppendPrintf("spec write dedupe/%d", i);
>       reg.principal() =
>-        mozilla::ipc::ContentPrincipalInfo(mozilla::OriginAttributes(0, false), spec);
>+        mozilla::ipc::ContentPrincipalInfo(mozilla::OriginAttributes(0, false),
>+                                           spec, spec);
Passing spec here as originNoSuffix is weird. Could you use EmptyCString or some substring of the spec and test its handling


>@@ -210,24 +212,30 @@ PrincipalToPrincipalInfo(nsIPrincipal* a
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>   if (NS_WARN_IF(!uri)) {
>     return NS_ERROR_FAILURE;
>   }
> 
>-  nsAutoCString spec;
>+  nsCString spec;
>   rv = uri->GetSpec(spec);
>   if (NS_WARN_IF(NS_FAILED(rv))) {
>     return rv;
>   }
> 
>+  nsCString origin;
>+  rv = aPrincipal->GetOriginNoSuffix(origin);
Nit, could you rename the variable to originNoSuffix to make the code easier to read.
Using nsCString here is a bit unusual, but may in theory avoid extra memcpy (but not malloc/free), so fine.


> struct ContentPrincipalInfo
> {
>   OriginAttributes attrs;
>+
>+  // Origin is not simply part of the spec. For some URLs, such as Blob URL,
>+  // the origin is the origin of the content who has created it.
nit, should it be "originNoSuffix is the origin of the content which created it."

Comment 15

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 16

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 17

[Bobby Holley (:bholley)]

As discussed in bug 1340710 comment 60, please wait for Ehsan's changes and then rebase this on top of them.

Comment 18

[Andrea Marchesini [:baku]]

Comment on attachment 8842862 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo

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

Cleaning review request. Probably this patch has to be rewritten when bug 1340710 lands.

Comment 19

[(no longer active)]

Apologies for the bitrot...

Comment 20

[Andrea Marchesini [:baku]]

Attachment: part 1.1 - Setting OriginAttributes in FinishInit()

Comment 21

[Andrea Marchesini [:baku]]

Attachment: part 1.2 - originNoSuffix in FinishInit

Comment 22

[Andrea Marchesini [:baku]]

Attachment: part 1.3 - Origin passed as argument when a principal is created

Comment 23

[Andrea Marchesini [:baku]]

Attachment: part 1.4 - ContentPrincipalInfo with origin

This is actually the main goal of this bug.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: part 1.5 - ServiceWorkerRegistrar with origin

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 1.6 - fixing tests

Comment 26

[(no longer active)]

I'll review the dom parts after bholley looks at the caps bits.

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8844843 [details] [diff] [review]
part 1.1 - Setting OriginAttributes in FinishInit()

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

r=me with that fix.

::: caps/BasePrincipal.h
@@ +287,5 @@
>    virtual bool MayLoadInternal(nsIURI* aURI) = 0;
>    friend class ::nsExpandedPrincipal;
>  
> +  void
> +  DomainIsSet()

This name sure sounds like a predicate. How about renaming it to mHasExplicitDomain/SetHasExplicitDomain?

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8844844 [details] [diff] [review]
part 1.2 - originNoSuffix in FinishInit

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

::: caps/nsExpandedPrincipal.cpp
@@ +70,5 @@
> +
> +    nsAutoCString subOrigin;
> +    nsresult rv = aWhiteList.ElementAt(i)->GetOrigin(subOrigin);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return nullptr;

This should be infallible now. Please just assert success.

::: caps/nsNullPrincipal.cpp
@@ +87,5 @@
> +  nsAutoCString originNoSuffix;
> +  nsresult rv = mURI->GetSpec(originNoSuffix);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

This is infallible [1]. Please just assert success.

[1] http://searchfox.org/mozilla-central/rev/d4adaabd6d2f88be0d0b151e022c06f6554909da/caps/nsNullPrincipalURI.cpp#244

::: caps/nsPrincipal.cpp
@@ +115,5 @@
> +  nsAutoCString originNoSuffix;
> +  nsresult rv = GenerateOriginNoSuffixFromURI(mCodebase, originNoSuffix);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }

So, this part of the patch can break things which rely on creating a principal for non-well-formed URIs. However, given that the next patch does the whole nullprincipal-creation business, I'm ok with this transient bustage, so long as it all lands together.

::: js/xpconnect/src/Sandbox.cpp
@@ +1441,5 @@
>  
>      RefPtr<nsExpandedPrincipal> result =
>          nsExpandedPrincipal::Create(allowedDomains, attrs.ref());
> +    if (!result) {
> +      return false;

Given the other review comment, this should be infallible, so the null check should be replaced with a MOZ_ASSERT.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp
@@ +297,5 @@
>      principalAsArray.AppendElement(principal);
>      RefPtr<nsExpandedPrincipal> ep =
>          nsExpandedPrincipal::Create(principalAsArray,
>                                      principal->OriginAttributesRef());
> +    if (NS_WARN_IF(!ep)) {

This should go away too.

Comment 29

[Bobby Holley (:bholley)]

Comment on attachment 8844845 [details] [diff] [review]
part 1.3 - Origin passed as argument when a principal is created

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

::: caps/BasePrincipal.cpp
@@ +614,5 @@
>  
>  already_AddRefed<BasePrincipal>
> +BasePrincipal::CreateCodebasePrincipal(nsIURI* aURI,
> +                                       const OriginAttributes& aAttrs,
> +                                       const nsACString& aOriginNoSuffix)

Hm, I really don't like the overloading here. This overload should just generate aOriginNoSuffix through the normal mechanism, and the string overload should assert that the resulting origin string matches the original string.

@@ +637,5 @@
>        return nsNullPrincipal::Create(aAttrs);
>      }
>      RefPtr<BasePrincipal> concrete = Cast(principal);
> +    // XXXbaku: we should assert that aAttrs matches with
> +    // concrete.OriginAttributesRef();

Please either add this assertion or delete the comment.

@@ +649,5 @@
> +      // If the generation of the origin fails, we still want to have a valid
> +      // principal. Better to return a null principal here.
> +      return nsNullPrincipal::Create(aAttrs);
> +    }
> +  }

This is the big scary/exciting change. I'd like bz to sr this.

@@ +733,5 @@
>    mOriginNoSuffix = NS_Atomize(aOriginNoSuffix);
>  }
>  
> +/* static */ nsresult
> +BasePrincipal::GenerateOriginNoSuffixFromURI(nsIURI* aURI,

This is a lot of code with interesting blame, and I'd really rather not move it if we don't need to. Can you just leave it in nsPrincipal.cpp, as a static method on nsPrincipal that we can just call from BasePrincipal.cpp?

With this moved back, I'm assuming there are no other changes to this function.

::: caps/nsPrincipal.cpp
@@ +343,5 @@
>  
>    domain = do_QueryInterface(supports);
>  
> +  nsAutoCString originNoSuffix;
> +  rv = aStream->ReadCString(originNoSuffix);

We should definitely not change the serialization. If we _did_, we'd need to bump the CIDs to avoid data corruption, and bumping the CIDs would break everybody's SessionStorage when they upgrade.

Instead, we should just call GenerateOriginNoSuffixFromURI during deserialization.

::: ipc/glue/BackgroundUtils.cpp
@@ +88,5 @@
>        if (info.attrs().mAppId != nsIScriptSecurityManager::UNKNOWN_APP_ID) {
>          attrs = info.attrs();
>        }
> +      nsAutoCString originNoSuffix;
> +      rv = BasePrincipal::GenerateOriginNoSuffixFromURI(uri, originNoSuffix);

Err, why can't we just call CreateCodebasesPrincipal like before?

Comment 30

[Bobby Holley (:bholley)]

Boris, can you confirm that you're ok with minting an nsNullPrincipal instead of an nsPrincipal of GetOrigin fails?

Comment 31

[Bobby Holley (:bholley)]

Comment on attachment 8844846 [details] [diff] [review]
part 1.4 - ContentPrincipalInfo with origin

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

I'm probably not the right person to review this patch.

Comment 32

[Bobby Holley (:bholley)]

Oh, and Part 1.3 should also remove the null-checking and slow-patch fallback in BasePrincipal::FastEquals and friends.

Comment 33

[(no longer active)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)
> Boris, can you confirm that you're ok with minting an nsNullPrincipal
> instead of an nsPrincipal of GetOrigin fails?

BTW if we do that, bug 1344595 should be reverted.

Comment 34

[Boris Zbarsky [:bzbarsky]]

This seems fine, I think, but please document that CreateCodebasePrincipal may not create a codebase principal, both on BasePrincipal and on the security manager...

Comment 35

[Andrea Marchesini [:baku]]

Attachment: part 1.3 - Origin passed as argument when a principal is created

Comment 36

[Bobby Holley (:bholley)]

Comment on attachment 8845281 [details] [diff] [review]
part 1.3 - Origin passed as argument when a principal is created

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

r=me with those fixes

::: caps/BasePrincipal.h
@@ +249,5 @@
>    static BasePrincipal* Cast(nsIPrincipal* aPrin) { return static_cast<BasePrincipal*>(aPrin); }
> +
> +  // These following 3 methods may not create a codebase principal in case it's
> +  // not possible to generate a correct origin from the passed URI. If this
> +  // happens, a NullPrincipal is returned.

This really only applies to the URI-valued overload, not the one that takes a string. Please make that clearer.

@@ +258,5 @@
>    static already_AddRefed<BasePrincipal>
>    CreateCodebasePrincipal(nsIURI* aURI, const OriginAttributes& aAttrs);
> +
> +  static already_AddRefed<BasePrincipal>
> +  CreateCodebasePrincipal(nsIURI* aURI, const OriginAttributes& aAttrs,

Please rename this to CreateCodebasePrincipalInternal and make it private, updating the comment above.

::: ipc/glue/BackgroundUtils.cpp
@@ +97,5 @@
> +      principal = BasePrincipal::CreateCodebasePrincipal(uri, attrs,
> +                                                         originNoSuffix);
> +      if (NS_WARN_IF(!principal)) {
> +        return nullptr;
> +      }

Per IRC discussion, the changes to this file should be reverted.

Comment 37

[(no longer active)]

Comment on attachment 8844846 [details] [diff] [review]
part 1.4 - ContentPrincipalInfo with origin

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

::: caps/nsJSPrincipals.cpp
@@ +202,5 @@
> +
> +        MOZ_ASSERT(dummy == 0);
> +
> +        nsAutoCString originNoSuffix;
> +        originNoSuffix.SetLength(originNoSuffixLength);

You seem to be trusting a length value that comes from the structured clone data here which is untrusted, so if a compromised content process for example sends something really large (such as 0xffffffff) they can at the very least cause a crash here.

I think this needs to be a fallible allocation instead.

::: ipc/glue/PBackgroundSharedTypes.ipdlh
@@ +22,5 @@
>    OriginAttributes attrs;
> +
> +  // Origin is not simply part of the spec. For some URLs
> +  // the origin is the origin of the content who has created it.
> +  ContentPrincipalInfoOrigin originNoSuffix;

The comment here is not really sufficiently describing what this field is doing, and when it should be passed, and what it should be set to, and what the implications are.  The commit message is also saying nothing about what the commit is doing.  I'm minusing because of the length issue, but can you please describe what this patch is doing *in the commit message*?

In general I always have this issue when reading your patches, your commit messages are usually very brief and sometimes it's easy to quickly determine what the patch does from the code but sometimes it can take more time, and especially when dealing with security sensitive code like this, as a reviewer I'd very much rather the author of the patch tell me their intention explicitly than I guess it.  :-)

Thanks so much!

Comment 38

[(no longer active)]

Comment on attachment 8844847 [details] [diff] [review]
part 1.5 - ServiceWorkerRegistrar with origin

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

Hmm, the connection of this bug to service workers isn't obvious to me at all.  The reason behind the connection should be mentioned in the commit message after you explain what the patch is doing.  :-)  My guess is that you did this because of the change to ContentPrincipalInfo in the previous patch?  But I'm not sure, and I'm also not sure why that was necessary in the first place, so I'm more confused after looking at this part than before.

Comment 39

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #33)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #30)
> > Boris, can you confirm that you're ok with minting an nsNullPrincipal
> > instead of an nsPrincipal of GetOrigin fails?
> 
> BTW if we do that, bug 1344595 should be reverted.

And don't forget about this!

Comment 40

[Bobby Holley (:bholley)]

From IRC:

> <@baku>	bholley: I need CreateCodebasePrincipal(uri, attrs, origin) for the IPC ContentPrincipalInfo. I cannot make it private.

Why is this? The uri+attrs and origin are redundant with each other, and ContentPrincipal shouldn't be passing both (this is especially important in context of IPC hardening, since we'd need to verify that the two are consistent with each other if we passed both). The easiest is probably to have ContentPrincipal just use CreateCodebasePrincipal(origin).

Comment 42

[Andrea Marchesini [:baku]]

The main reason why I started this bug is that I need to have the origin out of ContentPrincipalInfo without recreating a full nsIPrincipal. nsIPrincipal is main-thread only, but ContentPrincipalInfo is not, but it doesn't contain the origin.

> Why is this? The uri+attrs and origin are redundant with each other,

They are not for the scope of this bug. This about a uri such as file:///tmp/foobar with attrs {}.
The origin is: file://UNIVERSAL_FILE_URI_ORIGIN and, if I want to retrieve origin from a COntentPrincipalInfo, I need to be on the main-thread.

What I think we can do, is to have ContentPrincipalInfo as spec + attrs + originNoSuffix and, when we use PrincipalInfoToPrincipal(), we do:

nsCOMPtr<nsIPrincipal> p = CreateCodebasePrincipal(ipc.spec, ipc.attrs);
MOZ_ASSERT(p.getOrigin().Equals(ipc.origin));

Comment 43

[Andrea Marchesini [:baku]]

Attachment: part 1.4 - ContentPrincipalInfo with origin

ContentPrincipalInfo must contain the originNoSuffix in order to expose this value without recreating the nsIPrincipal. nsIPrincipal is main-thread only, but ContentPrincipalInfo can be used in any thread.

Note that the origin is not used directly in the generation of the nsIPrincipal but it's used only for an additional security check.

I also introduced the setLength() in any string allocated from an IPC message in the creation of the nsJSPrincipal.

Comment 44

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini [:baku] from comment #42)
> The main reason why I started this bug is that I need to have the origin out
> of ContentPrincipalInfo without recreating a full nsIPrincipal.

Ok, that seems reasonable. But that doesn't mean we need to pass both the URI and the origin when we _do_ recreate the full nsIPrincipal, right?

> nsIPrincipal
> is main-thread only, but ContentPrincipalInfo is not, but it doesn't contain
> the origin.
> 
> > Why is this? The uri+attrs and origin are redundant with each other,
> 
> They are not for the scope of this bug. This about a uri such as
> file:///tmp/foobar with attrs {}.
> The origin is: file://UNIVERSAL_FILE_URI_ORIGIN and, if I want to retrieve
> origin from a COntentPrincipalInfo, I need to be on the main-thread.

Ok. Keep in mind that the non-strict file:// URI is an unsupported configuration, so I don't care too much about it. But still good not to break it if we can.

> 
> What I think we can do, is to have ContentPrincipalInfo as spec + attrs +
> originNoSuffix and, when we use PrincipalInfoToPrincipal(), we do:
> 
> nsCOMPtr<nsIPrincipal> p = CreateCodebasePrincipal(ipc.spec, ipc.attrs);
> MOZ_ASSERT(p.getOrigin().Equals(ipc.origin));

Yes, this is the approach I had in mind. With that, we can make the 3-argument overload private.

Comment 45

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

Will we be able to land this soon and uplift to 53, which is affected?  If not, are there any alternatives for 53?  Thanks!

Comment 46

[Andrea Marchesini [:baku]]

Yeah, let's move the complex part of this set of patches in a separate bug, and let's keep here the ContentPrincipalInfo with origin only.

Comment 47

[Andrea Marchesini [:baku]]

Attachment: part 1 - quick fix

Comment 48

[(no longer active)]

Sorry for my review lag on this one.  :(

Comment 49

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

Comment on attachment 8847961 [details] [diff] [review]
part 1 - quick fix


>+      if (info.originNoSuffix().type() == ContentPrincipalInfoOrigin::TnsCString) {
>+        nsAutoCString originNoSuffix;
>+        rv = principal->GetOrigin(originNoSuffix);
>+        if (NS_WARN_IF(NS_FAILED(rv)) ||
>+            !info.originNoSuffix().get_nsCString().Equals(originNoSuffix)) {
>+          return nullptr;
>+        }
>+      }
Why is this ok? What if principal does have origin but info doesn't?
Or if this is ok, why do originNoSuffix test at all?
Maybe I'm missing something here.

 
>+union ContentPrincipalInfoOrigin
>+{
>+  nsCString;
>+  void_t;
>+};
So this hints origin (with suffix)


> struct ContentPrincipalInfo
> {
>   OriginAttributes attrs;
>+
>+  // nsIPrincipal.originNoSuffix can fail. In case this happens, this value
>+  // will be set to void_t.
>+  ContentPrincipalInfoOrigin originNoSuffix;
>+
but then this says no suffix. I think ContentPrincipalInfoOrigin should ContentPrincipalInfoOriginNoSuffix

Comment 50

[Andrea Marchesini [:baku]]

> Why is this ok? What if principal does have origin but info doesn't?

principal.origin can fail. My patches are not landed yet.

> Or if this is ok, why do originNoSuffix test at all?
> Maybe I'm missing something here.

The test is just for security reason.

> So this hints origin (with suffix)

I will rename it.

Comment 51

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

(In reply to Andrea Marchesini [:baku] from comment #50)
> > Why is this ok? What if principal does have origin but info doesn't?
> 
> principal.origin can fail. My patches are not landed yet.
That doesn't answer to my question


> 
> > Or if this is ok, why do originNoSuffix test at all?
> > Maybe I'm missing something here.
> 
> The test is just for security reason.
"just". Exactly. Why it is ok to ignore origin test in some case but not in others?

Comment 52

[Andrea Marchesini [:baku]]

Attachment: part 1 - quick fix

Comment 53

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

Comment on attachment 8848570 [details] [diff] [review]
part 1 - quick fix

>+#ifdef DEBUG
>+      if (info.originNoSuffix().type() == ContentPrincipalInfoOriginNoSuffix::TnsCString) {
>+        nsAutoCString originNoSuffix;
>+        rv = principal->GetOrigin(originNoSuffix);
>+        if (NS_WARN_IF(NS_FAILED(rv)) ||
>+            !info.originNoSuffix().get_nsCString().Equals(originNoSuffix)) {
>+          return nullptr;
We definitely don't want different results in debug builds.


Could you explain what you're trying to achieve with this patch?
We pass originNoSuffix in some cases, but not always. When is not passing it ok? What is the sent value used for?

Comment 54

[Andrea Marchesini [:baku]]

> Could you explain what you're trying to achieve with this patch?

We want a way to extract originNoSuffix from ContentPrincipalInfo in any thread. This is needed for dom/media (see the other patch) where we use the origin as key of an hashtable on a media thread.

Currently, principal.origin can fail and, if that happens I use void_t() in ContentPrincipalInfoOriginNoSuffix.
Otherwise, I use the string in ContentPrincipalInfoOriginNoSuffix. Now, just because principal.origin didn't fail when the principal was serialized, it should not fail also when it is deserialized: this is the reason why I introduced that check.

> We pass originNoSuffix in some cases, but not always. When is not passing it
> ok? What is the sent value used for?

As you know, there is another bug, with patches in review, for having principal.origin always succeeding, but, in order to have something easy to be backported, and quickly landed, here we have a subset of those patches.

Note that ContentPrincipalInfo follows the current setup of principal where origin could fail or not. I think this is acceptable.

I hope that this fix can be overwritten by the other patches soon.

Comment 55

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

ok, so this isn't used for security checks after all but some media stuff?

Anyhow,  the stuff in #ifdef DEBUG shouldn't be ifdef'ed, but perhaps crash if there is failure, and it needs a good comment why the check is there and what it is doing.

How does the media stuff work when there isn't originNoSuffix?

Comment 56

[Andrea Marchesini [:baku]]

> Anyhow,  the stuff in #ifdef DEBUG shouldn't be ifdef'ed, but perhaps crash
> if there is failure, and it needs a good comment why the check is there and
> what it is doing.

Yes, I like the crash approach.

> How does the media stuff work when there isn't originNoSuffix?

Yes. media works only with principal with a valid origin. That is used as key. But I can enforce it.

Comment 57

[Andrea Marchesini [:baku]]

Attachment: part 1 - origin as part of ContentPrincipalInfo

Comment 58

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

Comment on attachment 8849011 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo

>+++ b/ipc/glue/BackgroundUtils.cpp
>@@ -89,16 +89,25 @@ PrincipalInfoToPrincipal(const Principal
>         attrs = info.attrs();
>       }
>       principal = BasePrincipal::CreateCodebasePrincipal(uri, attrs);
>       rv = principal ? NS_OK : NS_ERROR_FAILURE;
>       if (NS_WARN_IF(NS_FAILED(rv))) {
>         return nullptr;
>       }
> 
>+      if (info.originNoSuffix().type() == ContentPrincipalInfoOriginNoSuffix::TnsCString) {
>+        nsAutoCString originNoSuffix;
>+        rv = principal->GetOrigin(originNoSuffix);
>+        if (NS_WARN_IF(NS_FAILED(rv)) ||
>+            !info.originNoSuffix().get_nsCString().Equals(originNoSuffix)) {
>+          MOZ_CRASH("If the origin was in the contentPrincipalInfo, it must be available when deserialized");
>+        }
>+      }
This needs some good comment why we check principal's origin only when type is CString, since one would think that
if type is void_t, also principal's origin should be empty


> struct ContentPrincipalInfo
> {
>   OriginAttributes attrs;
>+
>+  // nsIPrincipal.originNoSuffix can fail. In case this happens, this value
>+  // will be set to void_t.
>+  ContentPrincipalInfoOriginNoSuffix originNoSuffix;
Add a comment that one shouldn't use this for anything, something like
"temporary hack for media". Then once this all has been fixed properly the comment can be removed.

Comment 59

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54a1f0cb64d9
Introducing originNoSuffix as attribute in ContentPrincipalInfo, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/591c42f40cbe
Use of contentPrincipalInfo.origin in Media, r=jib

Comment 60

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for crashing e.g. in browser/components/originattributes/test/browser/browser_cacheAPI.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/324a8b9f44f4c1cc71022fa2e9279526374c5bfe
https://hg.mozilla.org/integration/mozilla-inbound/rev/aad74c60564368209d819fa264b7719101faacb4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=591c42f40cbe13f41ed8a8cbe9b0a8fde563b775&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log example: https://treeherder.mozilla.org/logviewer.html#?job_id=85045127&repo=mozilla-inbound

[task 2017-03-20T14:25:03.864671Z] 14:25:03     INFO - TEST-START | browser/components/originattributes/test/browser/browser_cacheAPI.js
[task 2017-03-20T14:25:05.475895Z] 14:25:05     INFO - GECKO(2198) | ExceptionHandler::GenerateDump cloned child 2285
[task 2017-03-20T14:25:05.478922Z] 14:25:05     INFO - GECKO(2198) | ExceptionHandler::SendContinueSignalToChild sent continue signal to child
[task 2017-03-20T14:25:05.481531Z] 14:25:05     INFO - GECKO(2198) | ExceptionHandler::WaitForContinueSignal waiting for continue signal...
[task 2017-03-20T14:25:06.566830Z] 14:25:06     INFO - TEST-INFO | Main app process: exit 11
[task 2017-03-20T14:25:06.570770Z] 14:25:06     INFO - Buffered messages logged at 14:25:03
[task 2017-03-20T14:25:06.571042Z] 14:25:06     INFO - Entering test bound 
[task 2017-03-20T14:25:06.571287Z] 14:25:06     INFO - Starting the test for first party isolation
[task 2017-03-20T14:25:06.571500Z] 14:25:06     INFO - Buffered messages finished
[task 2017-03-20T14:25:06.573234Z] 14:25:06    ERROR - TEST-UNEXPECTED-FAIL | browser/components/originattributes/test/browser/browser_cacheAPI.js | application terminated with exit code 11
[task 2017-03-20T14:25:06.576376Z] 14:25:06     INFO - runtests.py | Application ran for: 0:00:51.489177
[task 2017-03-20T14:25:06.578419Z] 14:25:06     INFO - zombiecheck | Reading PID log: /tmp/tmpIpZPiepidlog
[task 2017-03-20T14:25:06.581064Z] 14:25:06     INFO - ==> process 2198 launched child process 2217
[task 2017-03-20T14:25:06.583293Z] 14:25:06     INFO - zombiecheck | Checking for orphan process with PID: 2217
[task 2017-03-20T14:25:06.586284Z] 14:25:06     INFO - mozcrash Downloading symbols from: https://queue.taskcluster.net/v1/task/cNq8UdboQm2nEpZZ2uzL0g/artifacts/public/build/target.crashreporter-symbols.zip
[task 2017-03-20T14:25:21.179176Z] 14:25:21     INFO - mozcrash Copy/paste: /usr/local/bin/linux64-minidump_stackwalk /tmp/tmp19RG2N.mozrunner/minidumps/4bcd056e-1ebe-7a3b-1bc7009e-5d92f614.dmp /tmp/tmpI6W4mN
[task 2017-03-20T14:25:42.380541Z] 14:25:42     INFO - mozcrash Saved minidump as /home/worker/workspace/build/blobber_upload_dir/4bcd056e-1ebe-7a3b-1bc7009e-5d92f614.dmp
[task 2017-03-20T14:25:42.573648Z] 14:25:42     INFO - mozcrash Saved app info as /home/worker/workspace/build/blobber_upload_dir/4bcd056e-1ebe-7a3b-1bc7009e-5d92f614.extra
[task 2017-03-20T14:25:43.093120Z] 14:25:43     INFO - PROCESS-CRASH | browser/components/originattributes/test/browser/browser_cacheAPI.js | application crashed [@ mozilla::ipc::PrincipalInfoToPrincipal]
[task 2017-03-20T14:25:43.095752Z] 14:25:43     INFO - Crash dump filename: /tmp/tmp19RG2N.mozrunner/minidumps/4bcd056e-1ebe-7a3b-1bc7009e-5d92f614.dmp
[task 2017-03-20T14:25:43.097528Z] 14:25:43     INFO - Operating system: Linux
[task 2017-03-20T14:25:43.099228Z] 14:25:43     INFO -                   0.0.0 Linux 3.13.0-100-generic #147-Ubuntu SMP Tue Oct 18 16:48:51 UTC 2016 x86_64
[task 2017-03-20T14:25:43.100854Z] 14:25:43     INFO - CPU: x86
[task 2017-03-20T14:25:43.102593Z] 14:25:43     INFO -      GenuineIntel family 6 model 62 stepping 4
[task 2017-03-20T14:25:43.104333Z] 14:25:43     INFO -      1 CPU
[task 2017-03-20T14:25:43.105923Z] 14:25:43     INFO - 
[task 2017-03-20T14:25:43.107568Z] 14:25:43     INFO - GPU: UNKNOWN
[task 2017-03-20T14:25:43.109445Z] 14:25:43     INFO - 
[task 2017-03-20T14:25:43.111145Z] 14:25:43     INFO - Crash reason:  SIGSEGV
[task 2017-03-20T14:25:43.112905Z] 14:25:43     INFO - Crash address: 0x0
[task 2017-03-20T14:25:43.114556Z] 14:25:43     INFO - Process uptime: not available
[task 2017-03-20T14:25:43.116186Z] 14:25:43     INFO - 
[task 2017-03-20T14:25:43.117855Z] 14:25:43     INFO - Thread 0 (crashed)
[task 2017-03-20T14:25:43.119709Z] 14:25:43     INFO -  0  libxul.so!mozilla::ipc::PrincipalInfoToPrincipal [PBackgroundSharedTypes.h:591c42f40cbe : 77 + 0x0]
[task 2017-03-20T14:25:43.121458Z] 14:25:43     INFO -     eip = 0xf2b17f8c   esp = 0xffb876d0   ebp = 0xffb87788   ebx = 0xf6202154
[task 2017-03-20T14:25:43.123155Z] 14:25:43     INFO -     esi = 0xd2bc7b4c   edi = 0xffb8771c   eax = 0x08068b40   ecx = 0x00000019
[task 2017-03-20T14:25:43.124827Z] 14:25:43     INFO -     edx = 0xf50c1d06   efl = 0x00010246
[task 2017-03-20T14:25:43.126528Z] 14:25:43     INFO -     Found by: given as instruction pointer in context
[task 2017-03-20T14:25:43.128444Z] 14:25:43     INFO -  1  libxul.so!mozilla::dom::cache::PrincipalVerifier::VerifyOnMainThread [PrincipalVerifier.cpp:591c42f40cbe : 120 + 0xe]
[task 2017-03-20T14:25:43.130280Z] 14:25:43     INFO -     eip = 0xf35f1699   esp = 0xffb87790   ebp = 0xffb877e8   ebx = 0xf6202154
[task 2017-03-20T14:25:43.132026Z] 14:25:43     INFO -     esi = 0xd2bc7b30   edi = 0xffb877cc
[task 2017-03-20T14:25:43.133710Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.135515Z] 14:25:43     INFO -  2  libxul.so!mozilla::dom::cache::PrincipalVerifier::Run [PrincipalVerifier.cpp:591c42f40cbe : 100 + 0x9]
[task 2017-03-20T14:25:43.137225Z] 14:25:43     INFO -     eip = 0xf35f1802   esp = 0xffb877f0   ebp = 0xffb87808   ebx = 0xf6202154
[task 2017-03-20T14:25:43.138906Z] 14:25:43     INFO -     esi = 0xd2bc7b30   edi = 0xffb8784c
[task 2017-03-20T14:25:43.140465Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.142182Z] 14:25:43     INFO -  3  libxul.so!nsThread::ProcessNextEvent [nsThread.cpp:591c42f40cbe : 1269 + 0x9]
[task 2017-03-20T14:25:43.144135Z] 14:25:43     INFO -     eip = 0xf282d38e   esp = 0xffb87810   ebp = 0xffb87898   ebx = 0xf6202154
[task 2017-03-20T14:25:43.145789Z] 14:25:43     INFO -     esi = 0xf71308e0   edi = 0xffb8784c
[task 2017-03-20T14:25:43.147863Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.149940Z] 14:25:43     INFO -  4  libxul.so!NS_ProcessNextEvent [nsThreadUtils.cpp:591c42f40cbe : 389 + 0x10]
[task 2017-03-20T14:25:43.159144Z] 14:25:43     INFO -     eip = 0xf282de64   esp = 0xffb878a0   ebp = 0xffb878d8   ebx = 0xf6202154
[task 2017-03-20T14:25:43.160893Z] 14:25:43     INFO -     esi = 0xf170ceb0   edi = 0xf17261a0
[task 2017-03-20T14:25:43.162468Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.164231Z] 14:25:43     INFO -  5  libxul.so!mozilla::ipc::MessagePump::Run [MessagePump.cpp:591c42f40cbe : 96 + 0xc]
[task 2017-03-20T14:25:43.166175Z] 14:25:43     INFO -     eip = 0xf2b1c29b   esp = 0xffb878e0   ebp = 0xffb87928   ebx = 0xf6202154
[task 2017-03-20T14:25:43.168113Z] 14:25:43     INFO -     esi = 0xf170ceb0   edi = 0xf17261a0
[task 2017-03-20T14:25:43.170065Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.172112Z] 14:25:43     INFO -  6  libxul.so!MessageLoop::RunInternal [message_loop.cc:591c42f40cbe : 238 + 0x6]
[task 2017-03-20T14:25:43.174988Z] 14:25:43     INFO -     eip = 0xf2afea5a   esp = 0xffb87930   ebp = 0xffb87948   ebx = 0xf6202154
[task 2017-03-20T14:25:43.176686Z] 14:25:43     INFO -     esi = 0xf17261a0   edi = 0xf71308e0
[task 2017-03-20T14:25:43.178325Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.181119Z] 14:25:43     INFO -  7  libxul.so!MessageLoop::Run [message_loop.cc:591c42f40cbe : 231 + 0x7]
[task 2017-03-20T14:25:43.182847Z] 14:25:43     INFO -     eip = 0xf2afeb53   esp = 0xffb87950   ebp = 0xffb87978   ebx = 0xf6202154
[task 2017-03-20T14:25:43.184562Z] 14:25:43     INFO -     esi = 0xf17261a0   edi = 0xf71308e0
[task 2017-03-20T14:25:43.186365Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.188310Z] 14:25:43     INFO -  8  libxul.so!nsBaseAppShell::Run [nsBaseAppShell.cpp:591c42f40cbe : 156 + 0xe]
[task 2017-03-20T14:25:43.190327Z] 14:25:43     INFO -     eip = 0xf3ba9724   esp = 0xffb87980   ebp = 0xffb87998   ebx = 0xf6202154
[task 2017-03-20T14:25:43.193915Z] 14:25:43     INFO -     esi = 0xece9e830   edi = 0xf71308e0
[task 2017-03-20T14:25:43.195641Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.197423Z] 14:25:43     INFO -  9  libxul.so!nsAppStartup::Run [nsAppStartup.cpp:591c42f40cbe : 283 + 0x6]
[task 2017-03-20T14:25:43.199159Z] 14:25:43     INFO -     eip = 0xf46b8023   esp = 0xffb879a0   ebp = 0xffb879b8   ebx = 0xf6202154
[task 2017-03-20T14:25:43.200829Z] 14:25:43     INFO -     esi = 0xecdf07c0   edi = 0x00000077
[task 2017-03-20T14:25:43.202870Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.204784Z] 14:25:43     INFO - 10  libxul.so!XREMain::XRE_mainRun [nsAppRunner.cpp:591c42f40cbe : 4492 + 0x8]
[task 2017-03-20T14:25:43.206992Z] 14:25:43     INFO -     eip = 0xf4716064   esp = 0xffb879c0   ebp = 0xffb87ab8   ebx = 0xf6202154
[task 2017-03-20T14:25:43.208800Z] 14:25:43     INFO -     esi = 0xffb87a4c   edi = 0x00000077
[task 2017-03-20T14:25:43.210732Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.214119Z] 14:25:43     INFO - 11  libxul.so!XREMain::XRE_main [nsAppRunner.cpp:591c42f40cbe : 4670 + 0x9]
[task 2017-03-20T14:25:43.215898Z] 14:25:43     INFO -     eip = 0xf471662c   esp = 0xffb87ac0   ebp = 0xffb87b18   ebx = 0xf6202154
[task 2017-03-20T14:25:43.217596Z] 14:25:43     INFO -     esi = 0xffb87b50   edi = 0x00000000
[task 2017-03-20T14:25:43.219240Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.221072Z] 14:25:43     INFO - 12  libxul.so!XRE_main [nsAppRunner.cpp:591c42f40cbe : 4761 + 0x6]
[task 2017-03-20T14:25:43.225598Z] 14:25:43     INFO -     eip = 0xf471686d   esp = 0xffb87b20   ebp = 0xffb87c78   ebx = 0x08068488
[task 2017-03-20T14:25:43.227319Z] 14:25:43     INFO -     esi = 0xffb87b50   edi = 0xffb88db4
[task 2017-03-20T14:25:43.228902Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.230774Z] 14:25:43     INFO - 13  firefox!do_main [nsBrowserApp.cpp:591c42f40cbe : 236 + 0xf]
[task 2017-03-20T14:25:43.232517Z] 14:25:43     INFO -     eip = 0x0804cee8   esp = 0xffb87c80   ebp = 0xffb88cb8   ebx = 0x08068488
[task 2017-03-20T14:25:43.234343Z] 14:25:43     INFO -     esi = 0x00000005   edi = 0xffb88db4
[task 2017-03-20T14:25:43.236027Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.237727Z] 14:25:43     INFO - 14  firefox!main [nsBrowserApp.cpp:591c42f40cbe : 307 + 0xd]
[task 2017-03-20T14:25:43.242080Z] 14:25:43     INFO -     eip = 0x0804c6d8   esp = 0xffb88cc0   ebp = 0xffb88d08   ebx = 0x08068488
[task 2017-03-20T14:25:43.243805Z] 14:25:43     INFO -     esi = 0xffb88db4   edi = 0x00000005
[task 2017-03-20T14:25:43.245455Z] 14:25:43     INFO -     Found by: call frame info
[task 2017-03-20T14:25:43.247117Z] 14:25:43     INFO - 15  libc-2.15.so + 0x19533
[task 2017-03-20T14:25:43.248774Z] 14:25:43     INFO -     eip = 0xf740b533   esp = 0xffb88d10   ebp = 0x00000000
[task 2017-03-20T14:25:43.250558Z] 14:25:43     INFO -     Found by: previous frame's frame pointer
[task 2017-03-20T14:25:43.252313Z] 14:25:43     INFO - 16  firefox!__libc_csu_fini + 0x10
[task 2017-03-20T14:25:43.254003Z] 14:25:43     INFO -     eip = 0x080613d0   esp = 0xffb88d14   ebp = 0x00000000
[task 2017-03-20T14:25:43.255981Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.257991Z] 14:25:43     INFO - 17  libc-2.15.so + 0x19533
[task 2017-03-20T14:25:43.259942Z] 14:25:43     INFO -     eip = 0xf740b533   esp = 0xffb88d20   ebp = 0x00000000
[task 2017-03-20T14:25:43.262115Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.263917Z] 14:25:43     INFO - 18  libc-2.15.so + 0x1a4ff4
[task 2017-03-20T14:25:43.266489Z] 14:25:43     INFO -     eip = 0xf7596ff4   esp = 0xffb88d48   ebp = 0x00000000
[task 2017-03-20T14:25:43.268146Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.269690Z] 14:25:43     INFO - 19  firefox + 0x4974
[task 2017-03-20T14:25:43.272546Z] 14:25:43     INFO -     eip = 0x0804c974   esp = 0xffb88d70   ebp = 0x00000000
[task 2017-03-20T14:25:43.274131Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.275968Z] 14:25:43     INFO - 20  ld-2.15.so + 0x14660
[task 2017-03-20T14:25:43.277893Z] 14:25:43     INFO -     eip = 0xf7747660   esp = 0xffb88d78   ebp = 0x00000000
[task 2017-03-20T14:25:43.279862Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.282430Z] 14:25:43     INFO - 21  libc-2.15.so + 0x19449
[task 2017-03-20T14:25:43.284137Z] 14:25:43     INFO -     eip = 0xf740b449   esp = 0xffb88d7c   ebp = 0x00000000
[task 2017-03-20T14:25:43.286813Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.288436Z] 14:25:43     INFO - 22  ld-2.15.so + 0x20ff4
[task 2017-03-20T14:25:43.290089Z] 14:25:43     INFO -     eip = 0xf7753ff4   esp = 0xffb88d80   ebp = 0x00000000
[task 2017-03-20T14:25:43.292632Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.294187Z] 14:25:43     INFO - 23  firefox + 0x4974
[task 2017-03-20T14:25:43.296124Z] 14:25:43     INFO -     eip = 0x0804c974   esp = 0xffb88d88   ebp = 0x00000000
[task 2017-03-20T14:25:43.298156Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.302284Z] 14:25:43     INFO - 24  firefox!_start + 0x21
[task 2017-03-20T14:25:43.304273Z] 14:25:43     INFO -     eip = 0x0804c995   esp = 0xffb88d90   ebp = 0x00000000
[task 2017-03-20T14:25:43.308289Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.310418Z] 14:25:43     INFO - 25  firefox!SSE2Check [nsBrowserApp.cpp:591c42f40cbe : 92 + 0x8]
[task 2017-03-20T14:25:43.312183Z] 14:25:43     INFO -     eip = 0x0804c643   esp = 0xffb88d94   ebp = 0x00000000
[task 2017-03-20T14:25:43.313853Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.315659Z] 14:25:43     INFO - 26  firefox!__libc_csu_fini + 0x10
[task 2017-03-20T14:25:43.318987Z] 14:25:43     INFO -     eip = 0x080613d0   esp = 0xffb88da0   ebp = 0xffb88db4
[task 2017-03-20T14:25:43.320713Z] 14:25:43     INFO -     Found by: stack scanning
[task 2017-03-20T14:25:43.322248Z] 14:25:43     INFO - 27  0xffb8a536
[task 2017-03-20T14:25:43.324089Z] 14:25:43     INFO -     eip = 0xffb8a536   esp = 0xffb88dbc   ebp = 0xffb8a4fd
[task 2017-03-20T14:25:43.325945Z] 14:25:43     INFO -     Found by: previous frame's frame pointer
[task 2017-03-20T14:25:43.328671Z] 14:25:43     INFO - 28  0x6f772f65
[task 2017-03-20T14:25:43.330329Z] 14:25:43     INFO -     eip = 0x6f772f65   esp = 0xffb8a505   ebp = 0x6d6f682f
[task 2017-03-20T14:25:43.332158Z] 14:25:43     INFO -     Found by: previous frame's frame pointer

Comment 61

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a13df4897979
Introducing originNoSuffix as attribute in ContentPrincipalInfo, r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/f21336240c86
Use of contentPrincipalInfo.origin in Media, r=jib

Comment 62

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fe17d74ce9e
Fix a test failure related to ContentPrincipalInfo.origin usage, r=me CLOSED TREE

Comment 63

[Bobby Holley (:bholley)]

(In reply to Andrea Marchesini [:baku] from comment #46)
> Yeah, let's move the complex part of this set of patches in a separate bug,
> and let's keep here the ContentPrincipalInfo with origin only.

What was the separate bug number?

Comment 64

[Andrea Marchesini [:baku]]

Bug 1347817

Comment 66

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

https://hg.mozilla.org/mozilla-central/rev/a13df4897979
https://hg.mozilla.org/mozilla-central/rev/f21336240c86
https://hg.mozilla.org/mozilla-central/rev/0fe17d74ce9e

Comment 67

[Ryan VanderMeulen [:RyanVM]]

Please request Aurora/Beta approval on this when you get a chance.

Comment 68

[Andrea Marchesini [:baku]]

Comment on attachment 8838083 [details] [diff] [review]
part 2 - Origin in MediaParent

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1320170
[User impact if declined]: see the description of the bug
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: not yet. I verified when writing the patch.
[Needs manual test from QE? If yes, steps to reproduce]:  yes. see the description of the bug.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: It's low because we introduce a new attribute in ContentPrincipalInfo for storing the origin. Just an extra field.
[String changes made/needed]: none

Comment 69

[Gerry Chang [:gchang]]

Comment on attachment 8838083 [details] [diff] [review]
part 2 - Origin in MediaParent

Fix a regression. Aurora54+ & Beta53+.

Comment 70

[Carsten Book [:Tomcat]]

the test failure fix cause a problem when uplifting to aurora.
Baku can you take a look, thanks!

grafting 406162:0fe17d74ce9e "Bug 1340163 - Fix a test failure related to ContentPrincipalInfo.origin usage, r=me CLOSED TREE"
merging dom/media/tests/mochitest/test_enumerateDevices.html
warning: conflicts while merging dom/media/tests/mochitest/test_enumerateDevices.html! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 71

[Andrea Marchesini [:baku]]

Tomcat, just do not apply the 3rd patch. That test was added just to check those patches. Only the first 2 patches are enough for aurora.

Comment 72

[Carsten Book [:Tomcat]]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/272ad82f806b577c2814679f62040493b0b33fee
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/7fc9362cb6a5cc5a53b323d2aa48adc70dab415c

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/1fe52840b0eaa4b2ca02f66446351640a2848704
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/d01bf6283507b6820bd0961173426d6c21ebfc50

Comment 73

[Emil Ghitta, QA [:emilghitta]]

I have reproduce the issue mentioned in comment 0, using an affected Firefox 54.0a1 (Build Id:20170216030210) build.

I have verified that the issue is not reproducible using Firefox 53.0b6 (Build Id:20170323090023), Firefox 54.0a2 (Build Id: 20170324004022) and Firefox 55.0a1 (Build Id: 20170323030203) using Windows 10 64bit.

Comment 74

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

(In reply to Andrea Marchesini [:baku] from comment #68)
> [Is this code covered by automated tests?]: no

Actually, bug 1348174 added an automated test for this.

Comment 75

[Andrea Marchesini [:baku]]

Comment on attachment 8838083 [details] [diff] [review]
part 2 - Origin in MediaParent

[Approval Request Comment]
User impact if declined:  See bug 1337418
Risk to taking this patch (and alternatives if risky): these patches landed in aurora and beta. No big risks. Maybe we need a rebase.
String or UUID changes made by this patch:  none

Comment 76

[Julien Cristau [:jcristau]]

Comment on attachment 8849011 [details] [diff] [review]
part 1 - origin as part of ContentPrincipalInfo

crash fix for esr52

Comment 77

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f3f2a995a239

Comment 78

[Andrei Vaida [:avaida]]

Emil, could you please verify this on 52.1esr as well?

Comment 79

[Emil Ghitta, QA [:emilghitta]]

I have verified that this issue is not reproducible on Firefox 52.1.0esr (Build Id: 20170410145022) using Windows 10 64bit.