[e10s] nsExternalHelperAppService fails when called from content process

RESOLVED FIXED

Status

()

Core
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: blassey, Assigned: blassey)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(fennec2.0a1+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

Created attachment 461854 [details] [diff] [review]
patch
Attachment #461854 - Flags: review?(benjamin)
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.
Blocks: 575750
tracking-fennec: --- → 2.0a1+

Comment 2

8 years ago
>+    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.
Created attachment 461857 [details] [diff] [review]
patch
Attachment #461854 - Attachment is obsolete: true
Attachment #461857 - Flags: review?(benjamin)
Attachment #461854 - Flags: review?(benjamin)

Updated

8 years ago
Attachment #461857 - Flags: review?(benjamin) → review+

Updated

8 years ago
Assignee: nobody → blassey.bugs
pushed with a simple build fix:
http://hg.mozilla.org/mozilla-central/rev/af956e9c02e3
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Comment 6

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

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
(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.
(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.
Created attachment 463544 [details] [diff] [review]
updated patch

This fixes the spelling and the build error.  Pushing to try-server now.
Attachment #461857 - Attachment is obsolete: true
Attachment #463544 - Flags: review?(mwu)
Created attachment 463552 [details] [diff] [review]
updated patch v2

sigh, somehow I generated the last patch against an old tree.
Attachment #463544 - Attachment is obsolete: true
Attachment #463552 - Flags: review?(mwu)
Attachment #463544 - Flags: review?(mwu)
Attachment #463552 - Flags: review?(mwu)
Attachment #463552 - Flags: review?(doug.turner)
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?
Attachment #463552 - Flags: review?(doug.turner) → review-
Created attachment 463989 [details] [diff] [review]
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??
Attachment #463989 - Flags: review?(doug.turner)
(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 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.
Attachment #463989 - Flags: review?(doug.turner) → review+
http://hg.mozilla.org/mozilla-central/rev/90eb6b7a1ead
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.