583542 - [e10s] nsExternalHelperAppService fails when called from content process

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

This blocks bug 575750, which is a fennec 2.0a1 blocker

This is related to bug 562408, but this patch does nothing to fix the chrome registry assertion noted in the description so I'm filing it as a separate bug.

Comment 2

[Josh Matthews [:jdm]]

>+    return NS_SUCCEEDED(extProtService->LoadURI(ourURI, nsnull));

Returning false here will cause the content process to die in the future, which I suspect is not what we want.

>+#include "mozilla/dom/ContentChild.h"

This needs #ifdef MOZ_IPC.

>+  if (mozilla::dom::ContentChild::GetSingleton())
>+    mozilla::dom::ContentChild::GetSingleton()->SendLoadURIExteneral(aURI);
>+

I expect you want to return early from this rather than continue on and execute the rest of the function.

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 4

[Matt Brubeck (:mbrubeck)]

pushed with a simple build fix:
http://hg.mozilla.org/mozilla-central/rev/af956e9c02e3

Comment 5

[Matt Brubeck (:mbrubeck)]

missed the build fix the first time:
http://hg.mozilla.org/mozilla-central/rev/86b0c51d199c

Comment 6

[Josh Matthews [:jdm]]

Since this is being backed out, let's take the time to fix the "Externeral" IPDL typo, yes?

Comment 7

[Matt Brubeck (:mbrubeck)]

This patched caused a linkage error:
libxul.so: hidden symbol `nsExternalHelperAppService::mLog' isn't defined

backed out:
http://hg.mozilla.org/mozilla-central/rev/92ab40761955
http://hg.mozilla.org/mozilla-central/rev/42249b0f1a56

Comment 8

[Matt Brubeck (:mbrubeck)]

(In reply to comment #7)
> This patched caused a linkage error:
> libxul.so: hidden symbol `nsExternalHelperAppService::mLog' isn't defined

...or not - it looks like the tree went green again before this patch was backed out, so maybe it was caused by something else.

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to comment #8)
> ...or not - it looks like the tree went green again before this patch was
> backed out, so maybe it was caused by something else.

Nevermind, the green was on a different branch.  Sorry for the noise.

Comment 10

[Matt Brubeck (:mbrubeck)]

Attachment: updated patch

This fixes the spelling and the build error.  Pushing to try-server now.

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: updated patch v2

sigh, somehow I generated the last patch against an old tree.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 463552 [details] [diff] [review]
updated patch v2

>
>+bool
>+ContentParent::RecvLoadURIExternal(const IPC::URI& uri)
>+{
>+    nsCOMPtr<nsIExternalProtocolService> extProtService (do_GetService(NS_EXTERNALPROTOCOLSERVICE_CONTRACTID));

Everywhere else in the code base, we test for null after getting this service.  Not sure if this is just because we test aggersively for failure, or if this contact really is optional.  I am leaning toward it being optional.  Please test here.

Also, drop the space between expProtService and the first (

>+    nsCOMPtr<nsIURI> ourURI(uri);
>+    extProtService->LoadURI(ourURI, nsnull);
>+    return true;
>+}
>+
>+


Extra line.

>+include $(topsrcdir)/config/config.mk
>+include $(topsrcdir)/ipc/chromium/chromium-config.mk
> include $(topsrcdir)/config/rules.mk

Weird.  Can you ask Khuey which line is off, if any.

>+#ifdef MOZ_LOGGING
>+#define FORCE_PR_LOG
>+#endif
>+

Why?  

Also, what about the other methods in nsExternalHelperAppService?  We probably should return failure+warn if they are called in the child process?  Silent failures suck.

Also, how about a unit test?

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

(In reply to comment #12)
> Comment on attachment 463552 [details] [diff] [review]
> updated patch v2
> 
> >
> >+bool
> >+ContentParent::RecvLoadURIExternal(const IPC::URI& uri)
> >+{
> >+    nsCOMPtr<nsIExternalProtocolService> extProtService (do_GetService(NS_EXTERNALPROTOCOLSERVICE_CONTRACTID));
> 
> Everywhere else in the code base, we test for null after getting this service. 
> Not sure if this is just because we test aggersively for failure, or if this
> contact really is optional.  I am leaning toward it being optional.  Please
> test here.
new patch tests for null and returns early (with true)

> >+include $(topsrcdir)/config/config.mk
> >+include $(topsrcdir)/ipc/chromium/chromium-config.mk
> > include $(topsrcdir)/config/rules.mk
> 
> Weird.  Can you ask Khuey which line is off, if any.

huh? what makes you think something is off?

> >+#ifdef MOZ_LOGGING
> >+#define FORCE_PR_LOG
> >+#endif
> >+
> 
> Why?  

this I can't answer authoritatively, but the comments here may give a clue (http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#43). Also note that I see this pattern frequently in the code.

Fwiw I had previously seen the link error that prompted the back out once, after I had updated my tree.  If I recall correctly a clobber cleared it up.

> Also, what about the other methods in nsExternalHelperAppService?  We probably
> should return failure+warn if they are called in the child process?  Silent
> failures suck.

I believe crowder's work on the download manager should address that (bug 580673).

> Also, how about a unit test?

It looks like http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/tests/unit/test_handlerService.js#179 should test this already. Do we not run unit tests with remote tabs turned on??

Comment 14

[Matt Brubeck (:mbrubeck)]

(In reply to comment #12)
> >+#ifdef MOZ_LOGGING
> >+#define FORCE_PR_LOG
> >+#endif
> >+
> 
> Why?

This define needs to match between nsExternalHelperService.{h,cpp} so that mLog is defined if and only if it is declared.

Since we need to include at least some IPC headers before other headers (basictypes.h must be included before prtypes.h), we can't rely on nsExternalHelperService.h being included in the .cpp file before prlog.h is included.

Putting the #define in the .cpp file before all the #includes seemed like the least bad solution - it's least likely to break the next time someone adds or changes #include lines in this file.

Comment 15

[Doug Turner (:dougt)]

Comment on attachment 463989 [details] [diff] [review]
patch

joel says that these existing unit tests will eventually run but not for some time.

blassey, can you create a ipc test for this?  Follow up if you like, but having ipc tests is a good thing.

Comment 16

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/90eb6b7a1ead