890570 - Wean Necko off passing around PBrowsers

Status: RESOLVED FIXED

(Core :: Networking, defect)

Comment 1

[David Zbarsky (:dzbarsky)]

Attachment: PCookieService doesn't need PBrowser

Comment 2

[David Zbarsky (:dzbarsky)]

Comment on attachment 771715 [details] [diff] [review]
PCookieService doesn't need PBrowser

Justin, I think this is what we discussed for the security check.

Comment 3

[David Zbarsky (:dzbarsky)]

Attachment: PTCPSocket constructor doesn't need PBrowser

Comment 4

[David Zbarsky (:dzbarsky)]

Attachment: PRemoteOpenFile Constructor doesn't need PBrowser

Comment 5

[Josh Matthews [:jdm]]

Comment on attachment 771715 [details] [diff] [review]
PCookieService doesn't need PBrowser

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

::: netwerk/ipc/NeckoParent.cpp
@@ +79,5 @@
> +                                 uint32_t* aAppId,
> +                                 bool* aInBrowserElement)
> +{
> +  if (!aContent) {
> +    return "missing required PBrowser argument";

Ahem.

@@ +96,5 @@
> +      return nullptr;
> +    }
> +  }
> +
> +  return GetValidatedAppInfo(aSerialized, browsers[0], aAppId, aInBrowserElement);

This will break in the presence of no browsers, such as xpcshell tests, yes?

Comment 6

[Justin Lebar (not reading bugmail)]

> Ahem.

Sorry, what's actually wrong with this?

Comment 7

[Josh Matthews [:jdm]]

The error message doesn't make any sense now :)

Comment 8

[David Zbarsky (:dzbarsky)]

(In reply to Josh Matthews [:jdm] from comment #5)
> Comment on attachment 771715 [details] [diff] [review]
> PCookieService doesn't need PBrowser
> 
> Review of attachment 771715 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/ipc/NeckoParent.cpp
> @@ +79,5 @@
> > +                                 uint32_t* aAppId,
> > +                                 bool* aInBrowserElement)
> > +{
> > +  if (!aContent) {
> > +    return "missing required PBrowser argument";
> 
> Ahem.
> 
> @@ +96,5 @@
> > +      return nullptr;
> > +    }
> > +  }
> > +
> > +  return GetValidatedAppInfo(aSerialized, browsers[0], aAppId, aInBrowserElement);
> 
> This will break in the presence of no browsers, such as xpcshell tests, yes?

Yup.  I forgot to qref this patch.

Comment 9

[David Zbarsky (:dzbarsky)]

Attachment: PCookieService doesn't need PBrowser

Comment 10

[Justin Lebar (not reading bugmail)]

Comment on attachment 771844 [details] [diff] [review]
PCookieService doesn't need PBrowser

Looks pretty sane to me.

>diff --git a/netwerk/cookie/CookieServiceParent.cpp b/netwerk/cookie/CookieServiceParent.cpp
>--- a/netwerk/cookie/CookieServiceParent.cpp
>+++ b/netwerk/cookie/CookieServiceParent.cpp

> MOZ_WARN_UNUSED_RESULT
> static bool
> GetAppInfoFromParams(const IPC::SerializedLoadContext &aLoadContext,
>-                     PBrowserParent* aBrowser,
>+                     CookieServiceParent* aCookieService,
>                      uint32_t& aAppId,
>                      bool& aIsInBrowserElement,
>                      bool& aIsPrivate)
> {

Nit: Maybe this should be a method, instead of a function.

>diff --git a/netwerk/ipc/NeckoParent.cpp b/netwerk/ipc/NeckoParent.cpp
>--- a/netwerk/ipc/NeckoParent.cpp
>+++ b/netwerk/ipc/NeckoParent.cpp

>@@ -69,16 +70,56 @@ PBOverrideStatusFromLoadContext(const Se
> const char*
> NeckoParent::GetValidatedAppInfo(const SerializedLoadContext& aSerialized,
>+                                 PContentParent* aContent,
>+                                 uint32_t* aAppId,
>+                                 bool* aInBrowserElement)
>+{
>+  if (UsingNeckoIPCSecurity()) {
>+    if (!aContent) {
>+      return "missing required PContent argument";
>+    }
>+    if (!aSerialized.IsNotNull()) {
>+      return "SerializedLoadContext from child is null";
>+    }
>+  }
>+
>+  *aAppId = NECKO_UNKNOWN_APP_ID;
>+  *aInBrowserElement = false;

Nit: Probably better to set these before returning an error.  It is an XPCOM
rule that outparams are initialized iff the method is successful, but (a) this
isn't XPCOM, and (b) people don't always follow the rules.

>+  const InfallibleTArray<PBrowserParent*>& browsers = aContent->ManagedPBrowserParent();
>+  for (uint32_t i = 0; i < browsers.Length(); i++) {
>+    // GetValidatedAppInfo returning null means we passed security checks.
>+    if (!GetValidatedAppInfo(aSerialized, browsers[i], aAppId, aInBrowserElement)) {
>+      return nullptr;
>+    }
>+  }
>+
>+  if (browsers.Length() == 0) {
>+    MOZ_ASSERT(!UsingNeckoIPCSecurity());

If a process is compromised, it could call this method even if it has no
PBrowsers, right?

>+    if (aSerialized.IsNotNull()) {
>+      *aAppId = aSerialized.mAppId;
>+      *aInBrowserElement = aSerialized.mIsInBrowserElement;
>+    } else {
>+      *aAppId = NECKO_NO_APP_ID;
>+    }
>+    return nullptr;

So then we should return nullptr here only if !UsingNeckoIPCSecurity(), I think?

>+  }
>+
>+  return GetValidatedAppInfo(aSerialized, browsers[0], aAppId, aInBrowserElement);

Maybe add a comment here that you're doing this only to get an error message?

Comment 11

[David Zbarsky (:dzbarsky)]

(In reply to Justin Lebar [:jlebar] from comment #10)
> Comment on attachment 771844 [details] [diff] [review]
> PCookieService doesn't need PBrowser
> 
> 
> If a process is compromised, it could call this method even if it has no
> PBrowsers, right?
> 

Right; we should kill it in that case.

> 
> >+  }
> >+
> >+  return GetValidatedAppInfo(aSerialized, browsers[0], aAppId, aInBrowserElement);
> 
> Maybe add a comment here that you're doing this only to get an error message?

Good call.  Although this will change in a later patch, at which point it won't be worth it.

Comment 12

[David Zbarsky (:dzbarsky)]

Attachment: PCookieService doesn't need PBrowser

Comment 13

[David Zbarsky (:dzbarsky)]

Attachment: PTCPSocket constructor doesn't need PBrowser

Comment 14

[David Zbarsky (:dzbarsky)]

Attachment: Stop using PBrowser for all other protocols

Comment 15

[Jason Duell]

Comment on attachment 772199 [details] [diff] [review]
PCookieService doesn't need PBrowser

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

Just a couple nits and what looks like a possible separate bug.

::: netwerk/ipc/NeckoParent.cpp
@@ +79,5 @@
> +                                 PContentParent* aContent,
> +                                 uint32_t* aAppId,
> +                                 bool* aInBrowserElement)
> +{
> +  MOZ_ASSERT(aContent);

Not sure assert is sufficient here.  We need to make sure we handle the case where a rogue child process could set aContent to null, i.e. we should check for null and return an error if so.  That check should go in the "if (UsingNeckoIPCSecurity)" block as it's ok to be null for xpcshell tests.

@@ +81,5 @@
> +                                 bool* aInBrowserElement)
> +{
> +  MOZ_ASSERT(aContent);
> +  *aAppId = NECKO_UNKNOWN_APP_ID;
> +  *aInBrowserElement = false;

Is there a reason to set these default values before checking UsingNeckoSecurity below?  In the version of this function that take PBrowser we do the reverse order.  Make them both the same?

@@ +100,5 @@
> +
> +  if (browsers.Length() == 0) {
> +    if (UsingNeckoIPCSecurity()) {
> +      static_cast<ContentParent*>(aContent)->KillHard();
> +      return "ContentParent does not have any PBrowsers";

Elsewhere in the code we assume that returning non-null will cause child to die, so no need to KillHard IIUC?  (as we chatted about in person, it looks from a glance like the codepath actually won't kill the child but *will* NS_RUNTIMEABORT the parent process!! via 

   http://mxr.mozilla.org/mozilla-central/source/ipc/glue/AsyncChannel.cpp#492

which calls the generated PNeckoParent::OnProcessingError function which does the abort? That seems like a bug we should fix.

Comment 16

[David Zbarsky (:dzbarsky)]

Attachment: Fix http auth prompts for nested content processes

Comment 17

[David Zbarsky (:dzbarsky)]

(In reply to Jason Duell (:jduell) from comment #15)
> Comment on attachment 772199 [details] [diff] [review]
> PCookieService doesn't need PBrowser

> Not sure assert is sufficient here.  We need to make sure we handle the case
> where a rogue child process could set aContent to null, i.e. we should check
> for null and return an error if so.  That check should go in the "if
> (UsingNeckoIPCSecurity)" block as it's ok to be null for xpcshell tests.
> 

As we discussed, this assert is not necessary so I will remove it.

> @@ +81,5 @@
> > +                                 bool* aInBrowserElement)
> > +{
> > +  MOZ_ASSERT(aContent);
> > +  *aAppId = NECKO_UNKNOWN_APP_ID;
> > +  *aInBrowserElement = false;
> 
> Is there a reason to set these default values before checking
> UsingNeckoSecurity below?  In the version of this function that take
> PBrowser we do the reverse order.  Make them both the same?
> 

Justin suggested moving those up so we have sane return values, just in case.  I will delete the PBrowser function in a later patch.

> @@ +100,5 @@
> > +
> > +  if (browsers.Length() == 0) {
> > +    if (UsingNeckoIPCSecurity()) {
> > +      static_cast<ContentParent*>(aContent)->KillHard();
> > +      return "ContentParent does not have any PBrowsers";
> 
> Elsewhere in the code we assume that returning non-null will cause child to
> die, so no need to KillHard IIUC?  (as we chatted about in person, it looks
> from a glance like the codepath actually won't kill the child but *will*
> NS_RUNTIMEABORT the parent process!! via 
> 

I'll take out the KillHard since it's not needed.  I need to investigate this some more because a protocol error definitely does not abort the parent process.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 772210 [details] [diff] [review]
Stop using PBrowser for all other protocols

>diff --git a/netwerk/ipc/NeckoParent.h b/netwerk/ipc/NeckoParent.h
>--- a/netwerk/ipc/NeckoParent.h
>+++ b/netwerk/ipc/NeckoParent.h

>@@ -49,16 +49,17 @@ public:
>    * Values from PBrowserParent are more secure, and override those set in
>    * SerializedLoadContext.
>    *
>    * Returns null if successful, or an error string if failed.
>    */
>   MOZ_WARN_UNUSED_RESULT
>   static const char*
>   CreateChannelLoadContext(PBrowserParent* aBrowser,
>+                           PContentParent* aContent,
>                            const SerializedLoadContext& aSerialized,
>                            nsCOMPtr<nsILoadContext> &aResult);

It's not obvious what aContent is, if not aBrowser->Manager().  Could youa dd a
comment?

Comment 19

[Honza Bambas (:mayhemer)]

Comment on attachment 776217 [details] [diff] [review]
Fix http auth prompts for nested content processes

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

next time please:
- add a short explanation what the patch is doing and why you have chosen your strategy to fix it
- add comments to your code, for sure to any IDL changes (if there is need)
- don't do any reinterpret_casts the way you do in this patch please :)
- test your patch

Persoanally I'd prefer to rather have more fake classes to pass information to consumers then add new methods to interfaces, as we've talked about it.  If you oppose, please explain why, maybe there is a reason, I just don't see it.

::: docshell/base/LoadContext.cpp
@@ +47,5 @@
>  NS_IMETHODIMP
> +LoadContext::GetNestedFrameId(uint64_t* aId)
> +{
> +  *aId = mNestedFrameId;
> +  return NS_OK;

add also MOZ_ENSURE_ARG(aId);

::: docshell/base/nsILoadContext.idl
@@ +48,5 @@
> +   * access to the topFrameElement.  Instead, we must use this id to send
> +   * messages. A return value of 0 signifies that this load context is not for
> +   * a nested frame.
> +   */
> +  readonly attribute unsigned long long nestedFrameId;

change IID of the interface!

::: dom/browser-element/BrowserElementPromptService.jsm
@@ +453,2 @@
>        if (!frame)
> +        return context.nestedFrameId;

Ones you return an object and ones an arbitrary id?

::: dom/ipc/PBrowser.ipdl
@@ +284,5 @@
> +    /**
> +     * Brings up the auth prompt dialog.
> +     * Called when this is the PBrowserParent for a nested remote iframe.
> +     */
> +    AsyncAuthPrompt(nsCString uri, nsString realm, uint64_t aCallbackId);

What is the aCallbackId param?

::: dom/ipc/TabParent.cpp
@@ +1576,5 @@
> +  NS_ConvertASCIItoUTF16 realmU(aRealm);
> +
> +  nsRefPtr<nsAuthInformationHolder> holder =
> +    new nsAuthInformationHolder(promptFlags, realmU,
> +                                nsDependentCString("basic"));

what is this "basic" here?  I think EmptyCString() is OK here.

@@ +1582,5 @@
> +  uint32_t level = nsIAuthPrompt2::LEVEL_NONE;
> +  nsCOMPtr<nsICancelable> dummy;
> +  nsresult rv =
> +    authPrompt->AsyncPromptAuth2(channel, mFrameElement, channel, nullptr,
> +                                 level, holder, getter_AddRefs(dummy));

Why do you need to pass mFrameElement through a new method when we've talked about providing a "fake" load context?  Then BrowserElementAuthPrompt.mFrameElement would normally work.

Since you didn't provide any explanation when adding the patch, I have to ask and I really am against need of a new method.

Also, I think the 4th param should be |this| instead of |nullptr|.

Did you even try whether this works?

@@ +1598,5 @@
> +  if (!gNeckoChild->SendOnAuthAvailable(mCallbackId,
> +                                        holder->User(),
> +                                        holder->Password(),
> +                                        holder->Domain())) {
> +    return NS_ERROR_FAILURE;

This failure I think goes just to nowhere.

::: dom/ipc/TabParent.h
@@ -49,5 @@
>  
>  class ContentDialogParent : public PContentDialogParent {};
>  
> -class TabParent : public PBrowserParent 
> -                , public nsITabParent 

No whitespace only changes on lines you are not modifying please.

::: netwerk/base/public/nsIAuthPrompt2.idl
@@ +105,5 @@
> +                                 in nsIDOMElement aTarget,
> +                                 in nsIAuthPromptCallback aCallback,
> +                                 in nsISupports aContext,
> +                                 in uint32_t level,
> +                                 in nsIAuthInformation authInfo);

Add comments!

This needs a better name then just add "2" suffix.

Change IID!

::: netwerk/ipc/NeckoChild.cpp
@@ +194,5 @@
> +                                              const nsCString& aUri,
> +                                              const nsString& aRealm,
> +                                              const uint64_t& aCallbackId)
> +{
> +  TabChild* tabChild = reinterpret_cast<TabChild*>(aNestedFrameId);

Wow!!

::: netwerk/ipc/NeckoParent.cpp
@@ +94,5 @@
>  
> +  ContentParent* cp = static_cast<ContentParent*>(aContent);
> +  if (cp->Opener()) {
> +    cp = cp->Opener();
> +  }

Can you add comments what happens here?

@@ +490,5 @@
> +                                 const nsString& aUser,
> +                                 const nsString& aPassword,
> +                                 const nsString& aDomain)
> +{
> +  nsIAuthPromptCallback* callback = reinterpret_cast<nsIAuthPromptCallback*>(aCallbackId);

????

@@ +504,5 @@
> +bool
> +NeckoParent::RecvOnAuthCancelled(const uint64_t& aCallbackId,
> +                                 const bool& aUserCancel)
> +{
> +  nsIAuthPromptCallback* callback = reinterpret_cast<nsIAuthPromptCallback*>(aCallbackId);

???? Man, this is really something :)

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +668,5 @@
> +
> +  NS_IMETHOD AsyncPromptAuth(nsIChannel* aChannel, nsIAuthPromptCallback* callback,
> +                             nsISupports*, uint32_t,
> +                             nsIAuthInformation* aInfo, nsICancelable**) {
> +    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);

Hmm.. as I understand GeckoProcessType_Default is the root process type, right?  But bellow I see you are sending something to mNeckoParent.  It doesn't make sense to me.  Am I missing something?  There are absolutely no comments in your code, so it's hard to figure it out...

@@ +672,5 @@
> +    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> +    nsCOMPtr<nsIURI> uri;
> +    aChannel->GetURI(getter_AddRefs(uri));
> +    nsAutoCString spec;
> +    uri->GetSpec(spec);

null-checks/rv-checks please.

@@ +705,5 @@
> +                                 void** aResult)
> +{
> +  nsIAuthPrompt2* prompt = new NestedFrameAuthPrompt(Manager(), mNestedFrameId);
> +  NS_ADDREF(prompt);
> +  *aResult = reinterpret_cast<void*>(prompt);

nsCOMPtr<nsIAuthPrompt> and prompt.forget(aResult) ?

Comment 20

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #19)
> Comment on attachment 776217 [details] [diff] [review]
> Fix http auth prompts for nested content processes
> 
> Review of attachment 776217 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> next time please:
> - add a short explanation what the patch is doing and why you have chosen
> your strategy to fix it
> - add comments to your code, for sure to any IDL changes (if there is need)
> - don't do any reinterpret_casts the way you do in this patch please :)
> - test your patch
> 

Would you like an interdiff for the next patch?
We need the reinterpret casts because we can't pass some classes through IPDL, but I guess I can just assign them id's instead of using the pointer value.

> Persoanally I'd prefer to rather have more fake classes to pass information
> to consumers then add new methods to interfaces, as we've talked about it. 
> If you oppose, please explain why, maybe there is a reason, I just don't see
> it.
> 

This seemed cleaner.  I can add more fake interfaces, making FakeChannel implement nsIInterfaceRequestor and nsILoadContext, it just seems like a lot of unnecessary boilerplate.


> ::: dom/browser-element/BrowserElementPromptService.jsm
> @@ +453,2 @@
> >        if (!frame)
> > +        return context.nestedFrameId;
> 
> Ones you return an object and ones an arbitrary id?
> 

Yes.  This function actually returns a boolean value.

> ::: dom/ipc/PBrowser.ipdl
> @@ +284,5 @@
> > +    /**
> > +     * Brings up the auth prompt dialog.
> > +     * Called when this is the PBrowserParent for a nested remote iframe.
> > +     */
> > +    AsyncAuthPrompt(nsCString uri, nsString realm, uint64_t aCallbackId);
> 
> What is the aCallbackId param?
> 

I will add comments everywhere.

> ::: dom/ipc/TabParent.cpp
> @@ +1576,5 @@
> > +  NS_ConvertASCIItoUTF16 realmU(aRealm);
> > +
> > +  nsRefPtr<nsAuthInformationHolder> holder =
> > +    new nsAuthInformationHolder(promptFlags, realmU,
> > +                                nsDependentCString("basic"));
> 
> what is this "basic" here?  I think EmptyCString() is OK here.
> 

I'm not sure what that argument does, but that's the value it had when I tested the normal http auth.

> @@ +1582,5 @@
> > +  uint32_t level = nsIAuthPrompt2::LEVEL_NONE;
> > +  nsCOMPtr<nsICancelable> dummy;
> > +  nsresult rv =
> > +    authPrompt->AsyncPromptAuth2(channel, mFrameElement, channel, nullptr,
> > +                                 level, holder, getter_AddRefs(dummy));
> 
> Why do you need to pass mFrameElement through a new method when we've talked
> about providing a "fake" load context?  Then
> BrowserElementAuthPrompt.mFrameElement would normally work.
> 

This will go away when I make FakeChannel implement nsILoadContext.

> 
> Also, I think the 4th param should be |this| instead of |nullptr|.
> 

The 4th param is the | nsISupports* aContext | which is unused.  FWIW, we pass nullptr in the current code as well.

> Did you even try whether this works?
> 

Yes, it works.

> @@ +1598,5 @@
> > +  if (!gNeckoChild->SendOnAuthAvailable(mCallbackId,
> > +                                        holder->User(),
> > +                                        holder->Password(),
> > +                                        holder->Domain())) {
> > +    return NS_ERROR_FAILURE;
> 
> This failure I think goes just to nowhere.
> 

That's probably ok.  If SendOnAuthAvailable failed, our IPC is broken and the process will get shut down.

> 
> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +668,5 @@
> > +
> > +  NS_IMETHOD AsyncPromptAuth(nsIChannel* aChannel, nsIAuthPromptCallback* callback,
> > +                             nsISupports*, uint32_t,
> > +                             nsIAuthInformation* aInfo, nsICancelable**) {
> > +    MOZ_ASSERT(XRE_GetProcessType() == GeckoProcessType_Default);
> 
> Hmm.. as I understand GeckoProcessType_Default is the root process type,
> right?  But bellow I see you are sending something to mNeckoParent.  It
> doesn't make sense to me.  Am I missing something?  There are absolutely no
> comments in your code, so it's hard to figure it out...
> 

Right.  mNeckoParent->SendFoo sends a message to the NeckoChild...

Comment 21

[Honza Bambas (:mayhemer)]

(In reply to David Zbarsky (:dzbarsky) from comment #20)
> Would you like an interdiff for the next patch?

I usually compare patches my self, so no need.

> We need the reinterpret casts because we can't pass some classes through
> IPDL, but I guess I can just assign them id's instead of using the pointer
> value.

Understand, I had a similar problem ones.  So I've created a registrar which gives objects per-session unique ids and you can find then find them by that id (a simple hash table..).

> This seemed cleaner.  I can add more fake interfaces, making FakeChannel
> implement nsIInterfaceRequestor and nsILoadContext, it just seems like a lot
> of unnecessary boilerplate.

As I say, rather more 'empty' classes then modify an interface.

> > Ones you return an object and ones an arbitrary id?
> > 
> 
> Yes.  This function actually returns a boolean value.

Aha!  Then use !! and document it.

> > what is this "basic" here?  I think EmptyCString() is OK here.
> > 
> 
> I'm not sure what that argument does, but that's the value it had when I
> tested the normal http auth.

Me neither :)  but "basic" really doesn't belong here.  EmptyCString() is OK here.

> > 
> > Also, I think the 4th param should be |this| instead of |nullptr|.
> > 
> 
> The 4th param is the | nsISupports* aContext | which is unused.  FWIW, we
> pass nullptr in the current code as well.
> 
> > Did you even try whether this works?
> > 
> 
> Yes, it works.

Ah, sorry!  I overlooked what the arguments are.  Callback is the 3rd arg and it is the channel that is actually the target.  So it's correct.

Comment 22

[David Zbarsky (:dzbarsky)]

Attachment: Fix http auth prompts for nested content processes r=mayhemer

Comment 23

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #21)
> (In reply to David Zbarsky (:dzbarsky) from comment #20)
> > Would you like an interdiff for the next patch?

> 
> Aha!  Then use !! and document it.
> 

Fixed locally.

> > > what is this "basic" here?  I think EmptyCString() is OK here.
> > > 
> > 
> > I'm not sure what that argument does, but that's the value it had when I
> > tested the normal http auth.
> 
> Me neither :)  but "basic" really doesn't belong here.  EmptyCString() is OK
> here.
> 

Ok, i'll use an empty string.

Comment 24

[Honza Bambas (:mayhemer)]

Comment on attachment 781001 [details] [diff] [review]
Fix http auth prompts for nested content processes r=mayhemer

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

r-

::: dom/browser-element/BrowserElementPromptService.jsm
@@ +447,2 @@
>        if (!frame)
> +        return context.nestedFrameId;

!!

::: dom/ipc/TabParent.cpp
@@ +1526,5 @@
> +  NS_IMETHOD GetLoadFlags(nsLoadFlags*) { return NS_ERROR_NOT_IMPLEMENTED; }
> +  NS_IMETHOD GetOriginalURI(nsIURI**) { return NS_ERROR_NOT_IMPLEMENTED; }
> +  NS_IMETHOD SetOriginalURI(nsIURI*) { return NS_ERROR_NOT_IMPLEMENTED; }
> +  NS_IMETHOD GetURI(nsIURI** aUri) {
> +    NS_ADDREF(*aUri = mUri);

NS_IF_ADDREF, since you can't check result of NS_NewURI()

@@ +1586,5 @@
> +TabParent::RecvAsyncAuthPrompt(const nsCString& aUri,
> +                               const nsString& aRealm,
> +                               const uint64_t& aCallbackId)
> +{
> +  nsCOMPtr<nsIPromptFactory> promptService = do_GetService("@mozilla.org/prompter;1");

what is this for here?

@@ +1590,5 @@
> +  nsCOMPtr<nsIPromptFactory> promptService = do_GetService("@mozilla.org/prompter;1");
> +  nsIAuthPrompt2* authPrompt;
> +  GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL,
> +                NS_GET_IID(nsIAuthPrompt2),
> +                reinterpret_cast<void**>(&authPrompt));

You want to leak authPrompt instance?  nsCOMPtr, please.

@@ +1612,5 @@
> +NS_IMETHODIMP
> +FakeChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo)
> +{
> +  nsAuthInformationHolder* holder =
> +    static_cast<nsAuthInformationHolder*>(aAuthInfo);

You don't need to cast, nsIAuthInformation provides all info you need.

::: netwerk/ipc/NeckoChild.cpp
@@ +199,5 @@
> +  if (iter == sTabChildMap.end()) {
> +    MOZ_CRASH();
> +  }
> +  TabChild* tabChild = iter->second;
> +  tabChild->SendAsyncAuthPrompt(aUri, aRealm, aCallbackId);

iter->second?  I don't know std::, but it sounds odd.

::: netwerk/ipc/NeckoParent.cpp
@@ +94,5 @@
> +  ContentParent* cp = static_cast<ContentParent*>(aContent);
> +  if (cp->Opener()) {
> +    cp = cp->Opener();
> +  }
> +  const InfallibleTArray<PBrowserParent*>& browsers = cp->ManagedPBrowserParent();

Again, please explain, best with a comment in the code, why are you doing this change.

@@ +156,5 @@
>    // the common case for most xpcshell tests.
>    if (aSerialized.IsNotNull()) {
> +    if (aBrowser.type() == PBrowserOrId::TPBrowserParent) {
> +      nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(aBrowser.get_PBrowserParent());
> +      aResult = new LoadContext(aSerialized, tabParent->GetOwnerElement(), appId, inBrowser);

can we be sure tabParent is always non-null here?  maybe we do, just asking.

@@ +286,5 @@
>                    error);
>      return nullptr;
>    }
>  
> +  nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(browser.get_PBrowserParent());

could we get here with id-only?

@@ +479,5 @@
>    nsHTMLDNSPrefetch::CancelPrefetch(hostname, flags, reason);
>    return true;
>  }
>  
> +std::map<uint64_t, nsIAuthPromptCallback*> sCallbackMap;

See comments on std::map usage bellow...

@@ +498,5 @@
> +  holder->SetUsername(aUser);
> +  holder->SetPassword(aPassword);
> +  holder->SetDomain(aDomain);
> +  callback->OnAuthAvailable(nullptr, holder);
> +  return true;

few blank lines on appropriate places would make this code more readable.

::: netwerk/ipc/NeckoParent.h
@@ +13,5 @@
>  
>  namespace mozilla {
>  namespace net {
>  
> +extern std::map<uint64_t, nsIAuthPromptCallback*> sCallbackMap;

The maps (or tables) should be part of necko parent class, I think.  But this is entirely up to you.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +800,5 @@
>  //-----------------------------------------------------------------------------
>  // HttpChannelChild::nsIChildChannel
>  //-----------------------------------------------------------------------------
>  
> +std::map<uint64_t, TabChild*> sTabChildMap;

Use nsTHashtable or similar.  I don't know where from the trend of using std:: is coming from.  If there is a strong reason, then please explain.  Otherwise please use ns* classes for this.

BTW, this is a static initializer that is forbidden for performance reasons.

BTW2, how is the object removed from the map?  As I understand, the map doesn't hard-refer it, so it's possible that after TabChild goes away, in the map there is an invalid pointer, but the id can still be in the wild, right?

Just for curiosity, and also because others could benefit from having tabchildren accesible via an id, why doesn't every TabChild register in such a map automatically?  You can then expose a method on e.g. ContentChild (that from point of view of IPDL manages all TabChild instances) to search for a particular TabChild.  This is just a suggestion.

@@ +805,5 @@
> +
> +static uint64_t
> +GetTabChildId(TabChild* aTabChild)
> +{
> +  static uint64_t sId = 0;

maybe put this line under the if (aTabChild->Id() != 0) {} branch.  It will be slightly more efficient then.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +651,5 @@
>    mRedirectChannel = nullptr;
>    return NS_OK;
>  }
>  
> +class NestedFrameAuthPrompt MOZ_FINAL : public nsIAuthPrompt2

Please describe how is this class used.

@@ +662,5 @@
> +
> +  NS_DECL_ISUPPORTS
> +
> +  NS_IMETHOD PromptAuth(nsIChannel*, uint32_t, nsIAuthInformation*, bool*) {
> +    return NS_ERROR_NOT_IMPLEMENTED;

{ on a new line

@@ +668,5 @@
> +
> +  NS_IMETHOD AsyncPromptAuth(nsIChannel* aChannel, nsIAuthPromptCallback* callback,
> +                             nsISupports*, uint32_t,
> +                             nsIAuthInformation* aInfo, nsICancelable**) {
> +    static uint64_t callbackId;

should have = 0;

@@ +703,5 @@
> +  PNeckoParent* mNeckoParent;
> +  uint64_t mNestedFrameId;
> +};
> +
> +NS_IMPL_ISUPPORTS2(NestedFrameAuthPrompt, nsIAuthPrompt2, nsISupports);

NS_IMPL_ISUPPORTS1(NestedFrameAuthPrompt, nsIAuthPrompt2);

Comment 25

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #24)

> 
> @@ +1586,5 @@
> > +TabParent::RecvAsyncAuthPrompt(const nsCString& aUri,
> > +                               const nsString& aRealm,
> > +                               const uint64_t& aCallbackId)
> > +{
> > +  nsCOMPtr<nsIPromptFactory> promptService = do_GetService("@mozilla.org/prompter;1");
> 
> what is this for here?
> 

I think that's left over from something else I tried.

> @@ +1590,5 @@
> > +  nsCOMPtr<nsIPromptFactory> promptService = do_GetService("@mozilla.org/prompter;1");
> > +  nsIAuthPrompt2* authPrompt;
> > +  GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL,
> > +                NS_GET_IID(nsIAuthPrompt2),
> > +                reinterpret_cast<void**>(&authPrompt));
> 
> You want to leak authPrompt instance?  nsCOMPtr, please.
> 

nsCOMPtr won't work due to the reinterpret cast.  I added an NS_RELEASE call.


> @@ +1612,5 @@
> > +NS_IMETHODIMP
> > +FakeChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo)
> > +{
> > +  nsAuthInformationHolder* holder =
> > +    static_cast<nsAuthInformationHolder*>(aAuthInfo);
> 
> You don't need to cast, nsIAuthInformation provides all info you need.
> 

But then I need to use the ugly XPCOM methods.  I can mark nsIAuthinformation builtinclass so that this will always be safe.


> ::: netwerk/ipc/NeckoChild.cpp
> @@ +199,5 @@
> > +  if (iter == sTabChildMap.end()) {
> > +    MOZ_CRASH();
> > +  }
> > +  TabChild* tabChild = iter->second;
> > +  tabChild->SendAsyncAuthPrompt(aUri, aRealm, aCallbackId);
> 
> iter->second?  I don't know std::, but it sounds odd.
> 

find returns a pair of (key, value) so this is correct.

> ::: netwerk/ipc/NeckoParent.cpp
> @@ +94,5 @@
> > +  ContentParent* cp = static_cast<ContentParent*>(aContent);
> > +  if (cp->Opener()) {
> > +    cp = cp->Opener();
> > +  }
> > +  const InfallibleTArray<PBrowserParent*>& browsers = cp->ManagedPBrowserParent();
> 
> Again, please explain, best with a comment in the code, why are you doing
> this change.
> 

Sorry, this is from another patch in my queue.  It doesn't really make sense here.

> @@ +156,5 @@
> >    // the common case for most xpcshell tests.
> >    if (aSerialized.IsNotNull()) {
> > +    if (aBrowser.type() == PBrowserOrId::TPBrowserParent) {
> > +      nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(aBrowser.get_PBrowserParent());
> > +      aResult = new LoadContext(aSerialized, tabParent->GetOwnerElement(), appId, inBrowser);
> 
> can we be sure tabParent is always non-null here?  maybe we do, just asking.
> 

Yep, it's not marked uullable in IPDL so an attempt to send null will crash the child process.


> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +800,5 @@
> >  //-----------------------------------------------------------------------------
> >  // HttpChannelChild::nsIChildChannel
> >  //-----------------------------------------------------------------------------
> >  
> > +std::map<uint64_t, TabChild*> sTabChildMap;
> 

We're already doing this all over the place for things like
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/CompositableHost.cpp#123
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/CompositorParent.cpp#37
etc.
I think the reason for doing this is to lower the barrier to entry for new developers who are likely to be more familiar with STL than mozilla-specific things.
I would prefer to keep the std::map because I'm using the same thing for the dom/ code I'm adding and it would be nice to be consistent for these things.

Comment 26

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #24)
> Comment on attachment 781001 [details] [diff] [review]
> Fix http auth prompts for nested content processes r=mayhemer
> 
> Review of attachment 781001 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> @@ +286,5 @@
> >                    error);
> >      return nullptr;
> >    }
> > 
> > +  nsRefPtr<TabParent> tabParent = static_cast<TabParent*>(browser.get_PBrowserParent());
>
> could we get here with id-only?
>

Actually, we always pass the PBrowser for these constructors.  I'll fix that in a followup.

>
> BTW2, how is the object removed from the map?  As I understand, the map
> doesn't hard-refer it, so it's possible that after TabChild goes away, in
> the map there is an invalid pointer, but the id can still be in the wild,
> right?
>

You're right.  This should be a strong ref and the tab child can remove itself on ActorDestroy.

> Just for curiosity, and also because others could benefit from having
> tabchildren accesible via an id, why doesn't every TabChild register in such
> a map automatically?  You can then expose a method on e.g. ContentChild
> (that from point of view of IPDL manages all TabChild instances) to search
> for a particular TabChild.  This is just a suggestion.

It's not necessary.  Also, ContentChild will no longer manage all TabChild instances after some of my other patches.

Comment 27

[David Zbarsky (:dzbarsky)]

Attachment: Fix http auth prompts for nested content processes r=mayhemer

Comment 28

[Honza Bambas (:mayhemer)]

(In reply to David Zbarsky (:dzbarsky) from comment #25)
> > @@ +1590,5 @@
> > > +  nsCOMPtr<nsIPromptFactory> promptService = do_GetService("@mozilla.org/prompter;1");
> > > +  nsIAuthPrompt2* authPrompt;
> > > +  GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL,
> > > +                NS_GET_IID(nsIAuthPrompt2),
> > > +                reinterpret_cast<void**>(&authPrompt));
> > 
> > You want to leak authPrompt instance?  nsCOMPtr, please.
> > 
> 
> nsCOMPtr won't work due to the reinterpret cast.  I added an NS_RELEASE call.
> 

Not true.  QueryInterface also returns via void** and getter_AddRefs() can be used with it.

You will be ok with code like:

nsCOMPtr<nsIAuthPrompt2> authPrompt;
GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL,
              NS_GET_IID(nsIAuthPrompt2),
              getter_AddRefs(authPrompt));

> 
> > @@ +1612,5 @@
> > > +NS_IMETHODIMP
> > > +FakeChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo)
> > > +{
> > > +  nsAuthInformationHolder* holder =
> > > +    static_cast<nsAuthInformationHolder*>(aAuthInfo);
> > 
> > You don't need to cast, nsIAuthInformation provides all info you need.
> > 
> 
> But then I need to use the ugly XPCOM methods. 

Absolutely not an argument for me.

> I can mark
> nsIAuthinformation builtinclass so that this will always be safe.

So you will rather modify an interface and disallow its implementation by other consumers?  No.

> > @@ +800,5 @@
> > >  //-----------------------------------------------------------------------------
> > >  // HttpChannelChild::nsIChildChannel
> > >  //-----------------------------------------------------------------------------
> > >  
> > > +std::map<uint64_t, TabChild*> sTabChildMap;
> > 
> 
> We're already doing this all over the place for things like
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> CompositableHost.cpp#123

Hmm...  I have seen patches that got r- because of this.  I may lack some information on this.  This apparently needs someone else them me to decide.

Comment 29

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #28)
> (In reply to David Zbarsky (:dzbarsky) from comment #25)

> > 
> > nsCOMPtr won't work due to the reinterpret cast.  I added an NS_RELEASE call.
> > 
> 
> Not true.  QueryInterface also returns via void** and getter_AddRefs() can
> be used with it.
> 

Oh, I didn't know that. Thanks!

> > 
> > > @@ +1612,5 @@
> > > > +NS_IMETHODIMP
> > > > +FakeChannel::OnAuthAvailable(nsISupports *aContext, nsIAuthInformation *aAuthInfo)
> > > > +{
> > > > +  nsAuthInformationHolder* holder =
> > > > +    static_cast<nsAuthInformationHolder*>(aAuthInfo);
> > > 
> > > You don't need to cast, nsIAuthInformation provides all info you need.
> > > 
> > 
> > But then I need to use the ugly XPCOM methods. 
> 
> Absolutely not an argument for me.
> 
> > I can mark
> > nsIAuthinformation builtinclass so that this will always be safe.
> 
> So you will rather modify an interface and disallow its implementation by
> other consumers?  No.
> 

I just copied http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannelAuthProvider.cpp#1034
So any consumers who wish to provide an implementation of nsIAuthInformation are already broken.

> > > @@ +800,5 @@
> > > >  //-----------------------------------------------------------------------------
> > > >  // HttpChannelChild::nsIChildChannel
> > > >  //-----------------------------------------------------------------------------
> > > >  
> > > > +std::map<uint64_t, TabChild*> sTabChildMap;
> > > 
> > 
> > We're already doing this all over the place for things like
> > http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/
> > CompositableHost.cpp#123
> 
> Hmm...  I have seen patches that got r- because of this.  I may lack some
> information on this.  This apparently needs someone else them me to decide.

Could you ask additional review/feedback from someone who could decide this?  mattwoodrow r+ed my usage of it in gfx/dom but I don't think that'll count for you ;)

Comment 30

[David Zbarsky (:dzbarsky)]

(In reply to David Zbarsky (:dzbarsky) from comment #29)
> (In reply to Honza Bambas (:mayhemer) from comment #28)

> 
> I just copied
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannelAuthProvider.cpp#1034
> So any consumers who wish to provide an implementation of nsIAuthInformation
> are already broken.
> 

If you feel very strongly about this I can fix both of these, but given that nobody has tried to do this for many years it's probably not too important.

Comment 31

[David Zbarsky (:dzbarsky)]

Attachment: Fix http auth prompts for nested content processes r=mayhemer

Conflicts:
	netwerk/ipc/NeckoParent.cpp

Comment 32

[Honza Bambas (:mayhemer)]

Comment on attachment 782832 [details] [diff] [review]
Fix http auth prompts for nested content processes r=mayhemer

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

r=honzab but please address the last comments.

::: dom/browser-element/BrowserElementPromptService.jsm
@@ +447,3 @@
>        if (!frame)
> +        // This function returns a boolean value
> +        return !!context.nestedFrameId;

the if body in { } please

::: dom/ipc/TabParent.cpp
@@ +1580,5 @@
> +  nsCOMPtr<Element> mElement;
> +};
> +
> +NS_IMPL_ISUPPORTS4(FakeChannel, nsIChannel, nsIAuthPromptCallback,
> +                   nsIInterfaceRequestor, nsILoadContext);

You may want to add nsIRequest too

@@ +1596,5 @@
> +  uint32_t promptFlags = nsIAuthInformation::AUTH_HOST;
> +
> +  nsRefPtr<nsAuthInformationHolder> holder =
> +    new nsAuthInformationHolder(promptFlags, aRealm,
> +                                nsDependentCString("basic"));

still "basic" ?

@@ +1604,5 @@
> +  nsresult rv =
> +    authPrompt->AsyncPromptAuth(channel, channel, nullptr,
> +                                level, holder, getter_AddRefs(dummy));
> +
> +

Two blank lines

::: netwerk/base/public/nsIAuthPrompt2.idl
@@ +8,4 @@
>  interface nsIAuthPromptCallback;
>  interface nsIChannel;
>  interface nsICancelable;
> +interface nsIDOMElement;

Are these changes relevant to this patch?

::: netwerk/ipc/NeckoChild.cpp
@@ +201,5 @@
> +    MOZ_CRASH();
> +  }
> +  TabChild* tabChild = iter->second;
> +  tabChild->SendAsyncAuthPrompt(aUri, aRealm, aCallbackId);
> +  return true;

non-null check for tabChild please or return; in the if (iter == end()) {} branch

::: netwerk/ipc/NeckoParent.cpp
@@ +481,5 @@
>    nsHTMLDNSPrefetch::CancelPrefetch(hostname, flags, reason);
>    return true;
>  }
>  
> +std::map<uint64_t, nsIAuthPromptCallback*> sCallbackMap;

use nsCOMPtr<nsIAuthPromptCallback>

@@ +489,5 @@
> +                                 const nsString& aUser,
> +                                 const nsString& aPassword,
> +                                 const nsString& aDomain)
> +{
> +  nsIAuthPromptCallback* callback = sCallbackMap[aCallbackId];

nsCOMPtr<> here as well.

Comment 33

[David Zbarsky (:dzbarsky)]

(In reply to Honza Bambas (:mayhemer) from comment #32)
> Comment on attachment 782832 [details] [diff] [review]
> Fix http auth prompts for nested content processes r=mayhemer
> 
> Review of attachment 782832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=honzab but please address the last comments.
> 

Thanks!

> 
> ::: dom/ipc/TabParent.cpp
> @@ +1580,5 @@
> > +  nsCOMPtr<Element> mElement;
> > +};
> > +
> > +NS_IMPL_ISUPPORTS4(FakeChannel, nsIChannel, nsIAuthPromptCallback,
> > +                   nsIInterfaceRequestor, nsILoadContext);
> 
> You may want to add nsIRequest too

Won't hurt.

> 
> @@ +1596,5 @@
> > +  uint32_t promptFlags = nsIAuthInformation::AUTH_HOST;
> > +
> > +  nsRefPtr<nsAuthInformationHolder> holder =
> > +    new nsAuthInformationHolder(promptFlags, aRealm,
> > +                                nsDependentCString("basic"));
> 
> still "basic" ?
> 

Ah, I thought I fixed.

> 
> ::: netwerk/base/public/nsIAuthPrompt2.idl
> @@ +8,4 @@
> >  interface nsIAuthPromptCallback;
> >  interface nsIChannel;
> >  interface nsICancelable;
> > +interface nsIDOMElement;
> 
> Are these changes relevant to this patch?
> 

Nope, this was from an earlier iteration.

Comment 34

[David Zbarsky (:dzbarsky)]

Attachment: PRemoteOpenFile Constructor doesn't need PBrowser r=jduell

Comment 35

[David Zbarsky (:dzbarsky)]

Attachment: PTCPSocket constructor doesn't need PBrowser r=jduell

Comment 36

[David Zbarsky (:dzbarsky)]

Attachment: Stop using PBrowser for all other protocols r=jduell

Comment 37

[Jason Duell]

Comment on attachment 784656 [details] [diff] [review]
PTCPSocket constructor doesn't need PBrowser r=jduell

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

+r with nits.  No need for re-review if the changes are straightforward.

::: dom/network/src/TCPSocketParent.cpp
@@ +85,4 @@
>  {
>    // We don't have browser actors in xpcshell, and hence can't run automated
>    // tests without this loophole.
> +  if (!AssertAppProcessPermission(Manager()->Manager(), "tcp-socket")) {

Make sure this works with the existing xpcshell test (dom/network/tests/unit_ipc/test_tcpsocket_ipc.js).  If not you'll need to add "UsingNeckoIPCSecurity() &&" to the if clause.

(And if we don't need to add that, remove the comment above about the "loophole")

Comment 38

[Jason Duell]

Comment on attachment 784659 [details] [diff] [review]
Stop using PBrowser for all other protocols r=jduell

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

Good stuff.  Just a few nits, no need for re-review.

::: netwerk/ipc/NeckoParent.cpp
@@ +98,3 @@
>      *aAppId = tabParent->OwnOrContainingAppId();
>      *aInBrowserElement = aSerialized.IsNotNull() ? aSerialized.mIsInBrowserElement
>                                                   : tabParent->IsBrowserElement();

instead of setting aAppId/aInBrowserElement here, and *then* checking to see if they're correct, let's instead set some local variables, and only if we pass all the tests set aAppId/aInBrowserElement (right before we 'return nullptr').  Not a correctness issue, just reads more clearly.

@@ +122,5 @@
> +    return "App does not have permission";
> +  }
> +
> +  if (UsingNeckoIPCSecurity()) {
> +    return "ContentParent does not have any PBrowsers";

nit: invert the if condition here and put the "running xpcshell tests" code in the if body and return false.  Then do the error return by itself as the last line.

@@ +125,5 @@
> +  if (UsingNeckoIPCSecurity()) {
> +    return "ContentParent does not have any PBrowsers";
> +  }
> +
> +  // We are running tests

running *xpcshell* tests

Comment 39

[David Zbarsky (:dzbarsky)]

(In reply to Jason Duell (:jduell) from comment #37)
> Comment on attachment 784656 [details] [diff] [review]
> PTCPSocket constructor doesn't need PBrowser r=jduell
> 
> Review of attachment 784656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> +r with nits.  No need for re-review if the changes are straightforward.
> 
> ::: dom/network/src/TCPSocketParent.cpp
> @@ +85,4 @@
> >  {
> >    // We don't have browser actors in xpcshell, and hence can't run automated
> >    // tests without this loophole.
> > +  if (!AssertAppProcessPermission(Manager()->Manager(), "tcp-socket")) {
> 
> Make sure this works with the existing xpcshell test
> (dom/network/tests/unit_ipc/test_tcpsocket_ipc.js).  If not you'll need to
> add "UsingNeckoIPCSecurity() &&" to the if clause.
> 
> (And if we don't need to add that, remove the comment above about the
> "loophole")

Yep, we needed the UsingNeckoIPCSecurity check, good call.

https://hg.mozilla.org/integration/mozilla-inbound/rev/38657fcd2d1a
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae82c97dfa08
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc6bfc5b2b1f
https://hg.mozilla.org/integration/mozilla-inbound/rev/f99521bfd197

Comment 40

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/2c77b620ffe8 for a pair of xpcshell failures, all on linux, https://tbpl.mozilla.org/php/getParsedLog.php?id=26404709&tree=Mozilla-Inbound on 32bit and https://tbpl.mozilla.org/php/getParsedLog.php?id=26403025&tree=Mozilla-Inbound on 64bit.

Comment 41

[David Zbarsky (:dzbarsky)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7af1cdc56a10
https://hg.mozilla.org/integration/mozilla-inbound/rev/036bc0613a22
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab5c94e63711
https://hg.mozilla.org/integration/mozilla-inbound/rev/3faa29e53ac0

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/7af1cdc56a10
https://hg.mozilla.org/mozilla-central/rev/036bc0613a22
https://hg.mozilla.org/mozilla-central/rev/ab5c94e63711
https://hg.mozilla.org/mozilla-central/rev/3faa29e53ac0

Comment 43

[David Zbarsky (:dzbarsky)]

Kan-ru, is there anything else needed here?  I think you have other bugs filed for the GetValidatedAppInfo stuff.

Comment 44

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to David Zbarsky (:dzbarsky) from comment #43)
> Kan-ru, is there anything else needed here?  I think you have other bugs
> filed for the GetValidatedAppInfo stuff.

Right. The patch in this bug was landed in bug 879475 and followup in bug 1020157

(I wish we could mark "resolved fixed in other bugs")