1133189 - crash in mozalloc_abort(char const* const) | NS_DebugBreak | mozilla::net::PNeckoChild::Write(mozilla::ipc::PrincipalInfo const&, IPC::Message*)

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[Jim Mathies [:jimm]]

Currently #4 top content crasher - 

https://crash-stats.mozilla.com/report/list?product=Firefox&range_value=3&range_unit=days&date=2015-02-14&signature=mozalloc_abort%28char+const*+const%29+|+NS_DebugBreak+|+mozilla%3A%3Anet%3A%3APNeckoChild%3A%3AWrite%28mozilla%3A%3Aipc%3A%3APrincipalInfo+const%26%2C+IPC%3A%3AMessage*%29&version=Firefox%3A38.0a1

https://crash-stats.mozilla.com/report/index/7ef2aa01-8ba3-4355-a5e5-633e12150211


 	mozalloc.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp
1 	xul.dll 	NS_DebugBreak 	xpcom/base/nsDebugImpl.cpp
2 	xul.dll 	mozilla::net::PNeckoChild::Write(mozilla::ipc::PrincipalInfo const&, IPC::Message*) 	obj-firefox/ipc/ipdl/PNeckoChild.cpp
3 	xul.dll 	mozilla::net::PNeckoChild::Write(mozilla::net::HttpChannelOpenArgs const&, IPC::Message*) 	obj-firefox/ipc/ipdl/PNeckoChild.cpp
4 	xul.dll 	mozilla::net::PNeckoChild::Write(mozilla::net::HttpChannelCreationArgs const&, IPC::Message*) 	obj-firefox/ipc/ipdl/PNeckoChild.cpp
5 	xul.dll 	mozilla::net::PNeckoChild::SendPHttpChannelConstructor(mozilla::net::PHttpChannelChild*, mozilla::dom::PBrowserOrId const&, IPC::SerializedLoadContext const&, mozilla::net::HttpChannelCreationArgs const&) 	obj-firefox/ipc/ipdl/PNeckoChild.cpp
6 	xul.dll 	mozilla::net::HttpChannelChild::ContinueAsyncOpen()

Comment 1

[kalviskajaks]

Enhanced steam addon + e10s enabled + go to steam webpage = instant tab crash 
https://crash-stats.mozilla.com/report/index/c4effdc5-1dc3-4dda-bc8a-0419f2150214

Comment 2

[Jim Mathies [:jimm]]

Currently the #10 top content crasher, and flagged as a startup crash in crash stats.

Comment 3

[Jim Mathies [:jimm]]

steam addon crash - 

[Child 3756] ###!!! ABORT: unknown union type: file f:\mozilla\dbg\ipc\ipdl\PNeckoChild.cpp, line 3099

In this case, for an nsXMLHttpRequest initiated in the child, we try to serialize a principal which doesn't have a uri associated with it, and that causes an early return failure[1] which doesn't getr picked up in propagateLoadInfo [2], which causes an ipc write abort when the data is serialized.


[1] http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundUtils.cpp#91
[2] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#1420

crash stack:

xul.dll!NS_DebugBreak(unsigned int aSeverity, const char * aStr, const char * aExpr, const char * aFile, int aLine) Line 424	C++
xul.dll!mozilla::net::PNeckoChild::Write(const mozilla::ipc::PrincipalInfo & __v, IPC::Message * __msg) Line 3099	C++
xul.dll!mozilla::net::PNeckoChild::Write(const mozilla::net::HttpChannelOpenArgs & __v, IPC::Message * __msg) Line 4028	C++
xul.dll!mozilla::net::PNeckoChild::Write(const mozilla::net::HttpChannelCreationArgs & __v, IPC::Message * __msg) Line 3504	C++
xul.dll!mozilla::net::PNeckoChild::SendPHttpChannelConstructor(mozilla::net::PHttpChannelChild * actor, const mozilla::dom::PBrowserOrId & browser, const IPC::SerializedLoadContext & loadContext, const mozilla::net::HttpChannelCreationArgs & args) Line 303	C++
xul.dll!mozilla::net::HttpChannelChild::ContinueAsyncOpen() Line 1644	C++
xul.dll!mozilla::net::HttpChannelChild::AsyncOpen(nsIStreamListener * listener, nsISupports * aContext) Line 1529	C++
>	xul.dll!nsXMLHttpRequest::Send(nsIVariant * aVariant, const mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> & aBody) Line 3016	C++
xul.dll!nsXMLHttpRequest::Send(const mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> & aBody) Line 423	C++
xul.dll!nsXMLHttpRequest::Send(JSContext * __formal, mozilla::ErrorResult & aRv) Line 434	C++
xul.dll!nsXMLHttpRequest::Send(JSContext * aCx, const nsAString_internal & aString, mozilla::ErrorResult & aRv) Line 461	C++
xul.dll!mozilla::dom::XMLHttpRequestBinding::send(JSContext * cx, JS::Handle<JSObject *> obj, nsXMLHttpRequest * self, const JSJitMethodCallArgs & args) Line 709	C++
xul.dll!mozilla::dom::GenericBindingMethod(JSContext * cx, unsigned int argc, JS::Value * vp) Line 2537	C++

Comment 4

[Jim Mathies [:jimm]]

Jason, any thoughts here? This is a pretty common crash on the child side.

Comment 5

[Jim Mathies [:jimm]]

Or maybe the bug is we're missing a good principal for this request?

Comment 6

[Jason Duell]

Christoph: this seems likely to be from some code you've touched recently?

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Mhm, when does a Principal not have a URI (other than system) which we check in PropagateLoadInfo:
http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundUtils.cpp#115

Unfortunately, I can't reproduce at the moment, any chance someone could provide some detailed steps to reproduce?

I can debug and figure out what's going on after that! Potentially it's some addon created channel and something is off with the principal in the loadInfo.

Comment 8

[Tanvi Vyas[:tanvi]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #7)
> Mhm, when does a Principal not have a URI (other than system) which we check
> in PropagateLoadInfo:
> http://mxr.mozilla.org/mozilla-central/source/ipc/glue/BackgroundUtils.
> cpp#115
> 

When it's an expanded principal?
https://developer.mozilla.org/en-US/docs/Components.utils.Sandbox#Expanded_principal

Comment 9

[Jim Mathies [:jimm]]

The only str that works at this point uses the steam add-on to trigger:

1) install the enhanced steam addon
2) restart
3) make sure e10s is on
4) attach a suitable debugger to the content process
5) visit store.steampowered.com

after about 5 seconds of loading the content process should abort.

Comment 10

[Tanvi Vyas[:tanvi]]

(In reply to Jim Mathies [:jimm] from comment #9)
> The only str that works at this point uses the steam add-on to trigger:
> 
> 1) install the enhanced steam addon
Can you provide a link to this addon?

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

You are right Tanvi, it is an expanded principal:

> LoadingPrincipal: expanded
> SecurityFlags: 1
> ContentPolicyType: 11
> InnerWindowID: 0


The question now is, how to we deal with expanded principals?
Pinging Jonas now.

Comment 12

[Jim Mathies [:jimm]]

Triage note - this is a problem for addons and content process stability, doesn't impact firefox proper. I think we want to get this fixed before aurora uplift.

Comment 13

[kalviskajaks]

(In reply to Tanvi Vyas [:tanvi] from comment #10)
> Can you provide a link to this addon?

http://firefox.enhancedsteam.com/latest/enhanced-steam.xpi

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1133189_extend_principalinfo.patch

Jonas, Ben, it would be great if the PrincipalInfo could also handle expanded principals. The attached patch seems to perform what we want.

A few questions/comments:
* Exporting nsPrincipal.h, nsScriptSecurityManager.h is not that great, but there is no other way (at the moment) to create an expanded principal as to call the constructor directly which lives in nsPrincipal.h. Exporting nsPrincipal.h further forces us also to export nsScriptSecurityManager.h.

* I am not that happy about the reinterpret_cast; is there a better option? The expanded Principal only inherits nsISupports, but not nsIPrincipal, hence the reinterpretation.

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1133189_extend_principalinfo.patch

Chatted with Jonas in person - we should use do_QueryInterface instead of the reinterpret_cast. Other than that this is ready for a final review round.

Jonas, Ben is out for some time, can you take a first look?

Comment 16

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

Comment on attachment 8576196 [details] [diff] [review]
bug_1133189_extend_principalinfo.patch

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

::: ipc/glue/BackgroundUtils.cpp
@@ +171,5 @@
> +
> +    expanded->GetWhiteList(&whitelist);
> +    for (uint32_t i = 0; i < whitelist->Length(); i++) {
> +      // get the uri from the principal
> +      rv = (*whitelist)[i]->GetURI(getter_AddRefs(wlURI));

As far as I can tell, there's nothing guaranteeing that the principal inside the expanded-principal has a URI. Or that it is a codebase-principal.

Instead recursively call PrincipalToPrincipalInfo here.

And do the same on the PrincipalInfoToPrincipal side of course.

::: ipc/glue/PBackgroundSharedTypes.ipdlh
@@ +21,5 @@
>  { };
>  
> +struct ExpandedPrincipalInfo
> +{
> +  nsCString[] whitelist;

Make this be |PrincipalInfo[] whitelist;| Hopefully that should compile.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1133189_extend_principalinfo.patch

Jonas, you are right, we should call it recursively - here we go.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8577094 [details] [diff] [review]
bug_1133189_extend_principalinfo.patch

Ben, can you please also take a look at this one?

Comment 19

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

Comment on attachment 8577094 [details] [diff] [review]
bug_1133189_extend_principalinfo.patch

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

::: ipc/glue/BackgroundUtils.cpp
@@ +84,5 @@
> +      const ExpandedPrincipalInfo& info =
> +        aPrincipalInfo.get_ExpandedPrincipalInfo();
> +
> +      nsTArray< nsCOMPtr<nsIPrincipal> > whitelist;
> +      nsCOMPtr<nsIPrincipal> wlPrincipal;

Move wlPrincipal inside the loop

@@ +97,5 @@
> +      }
> +
> +      nsCOMPtr<nsIExpandedPrincipal> expandedPrincipal =
> +        new nsExpandedPrincipal(whitelist);
> +      if (!expandedPrincipal) {

Remove the nullcheck. If 'new' fails our allocator will crash.

@@ +161,5 @@
> +    nsTArray< nsCOMPtr<nsIPrincipal> >* whitelist;
> +    expanded->GetWhiteList(&whitelist);
> +
> +    for (uint32_t i = 0; i < whitelist->Length(); i++) {
> +      rv = PrincipalToPrincipalInfo((*whitelist)[i], &info);

Remove the 'info' temporary, and change this to:

rv = PrincipalToPrincipalInfo(whitelist->ElementAt(i),
                              whitelistInfo.AppendElement());

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Thanks Jonas, will wait for Ben's review and then incorporate your changes.

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8577094 [details] [diff] [review]
bug_1133189_extend_principalinfo.patch

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

Sorry for the delay!

::: caps/moz.build
@@ +18,5 @@
>      'nsJSPrincipals.h',
>      'nsNullPrincipal.h',
>      'nsNullPrincipalURI.h',
> +    'nsPrincipal.h',
> +    'nsScriptSecurityManager.h',

I'm not sure this is a great idea... How about you just add 'caps' to ipc/glue/moz.build in the LOCAL_INCLUDES section? That way these files don't get exposed everywhere.

::: ipc/glue/BackgroundUtils.cpp
@@ +83,5 @@
> +    case PrincipalInfo::TExpandedPrincipalInfo: {
> +      const ExpandedPrincipalInfo& info =
> +        aPrincipalInfo.get_ExpandedPrincipalInfo();
> +
> +      nsTArray< nsCOMPtr<nsIPrincipal> > whitelist;

Nit: Lose the space padding here and below

@@ +96,5 @@
> +        whitelist.AppendElement(wlPrincipal);
> +      }
> +
> +      nsCOMPtr<nsIExpandedPrincipal> expandedPrincipal =
> +        new nsExpandedPrincipal(whitelist);

Just let this be |nsRefPtr<nsExpandedPrincipal>| and then you don't need to QI when you store into |principal| (you may have to static_cast, but that's all).

@@ +158,5 @@
> +    nsTArray<PrincipalInfo> whitelistInfo;
> +    PrincipalInfo info;
> +
> +    nsTArray< nsCOMPtr<nsIPrincipal> >* whitelist;
> +    expanded->GetWhiteList(&whitelist);

Since this XPCOM method might change someday it is probably safer to make this"

  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(expanded->GetWhiteList(&whitelist)));

@@ +169,5 @@
> +      // append that spec to the whitelist
> +      whitelistInfo.AppendElement(info);
> +    }
> +
> +    *aPrincipalInfo = ExpandedPrincipalInfo(whitelistInfo);

pass |Move(whitelistInfo)| here to avoid copying.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1133189_extend_principalinfo.patch

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #21)
> Sorry for the delay!

No problem, thanks for the detailed feedback!

> Just let this be |nsRefPtr<nsExpandedPrincipal>| and then you don't need to
> QI when you store into |principal| (you may have to static_cast, but that's
> all).

I don't think we can remove the QI here, it's not C++ inheritance model, it's our own, isn't it? See:
http://mxr.mozilla.org/mozilla-central/source/caps/nsIPrincipal.idl#256

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1133189_extend_principalinfo.patch

nsRefPtr<nsExpandedPrincipal> instead of
nsRefPtr<nsIExpandedPrincipal> :-)

Thanks Ben - works just fine now!

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8bd316ad33c6

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out under the suspicion of having caused a spike in OSX timeouts in browser_frameworker_sandbox.js.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6dae39f2f807

https://treeherder.mozilla.org/logviewer.html#?job_id=8501722&repo=mozilla-inbound

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Wasn't at fault. Re-landed.
https://hg.mozilla.org/integration/mozilla-inbound/rev/407ad54f6e30

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/407ad54f6e30

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Jim, I tested manually and everything seems to be fine. Can you confirm? Also, I think this patch needs to be uplifted, right?

Comment 29

[Jim Mathies [:jimm]]

No more steam crashes, looks good.