562444 - e10s HTTP: implement necko logic needed to handle downloads in chrome

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

From my understanding of the needs of the download manager in e10s:

Content will receive OnStartRequest and decide whether the channel's content can be rendered.  If not, it needs to tell chrome to download it via the download manager.

Any data from SendOnDataAvailable that is already in flight will need to be sent back to chrome to become part of the download.  Not sure of the cleanest way to handle this.

Comment 1

[Jason Duell]

From some discussion on IRC we have a possible simpler architecture:

Hopefully we can determine in chrome (in HttpChannelParent::OnStartRequest, or perhaps in nsHttpChannel) whether we're going to download on not based on the MIME type.  If so, we can hand off the nsHttpChannel to the download manager.

Issues:

1) We'd need to do this only for top-level loads (I will open a bug to provide a flag in HttpChannelParent for checking this, as I think we need to determine it in the child and pass the flag in AsyncOpen)

2) We'll need to figure out what happens to the content side of the channel.  Presumably it needs to get some sort of msg indicating that the DL mgr will be handling the channel.  I don't know the URILoader/DocShell (or whatever) code well enough off the top of my head to know what that should look like from the necko client's perspective.

Comment 2

[Jason Duell]

biesi suspect we may also download unknown MIME types for <iframe>s.  If true, presumably we need to a flag for this from the content process as well.

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

I believe that for the child, you can just cancel(NS_BINDING_RETARGETED) the channel and uriloader/docshell will handle that correctly (make sure to send onStartRequest/onStopRequest, of course).

For the parent, you may want to refactor uriloader so that you can add a method that tells you whether a load would go to the nsExternalHelperService... though maybe that doesn't work so well. The problem is that in our current API, each nsIURIContentListener (e.g. each docshell) can just say "I don't want this load", and if all of them say that you get a download.

Comment 4

[Jason Duell]

So biesi/bz agree that we need to handle downloads for <iframe>s as well as top-level loads.  This winds up meaning that we don't need a new "IsTopLevelLoad" function:  just call GetLoadFlags() on the nsHttpChannel, and if LOAD_DOCUMENT_URI is not set, the download manager should not be used (and if it is, we need to try to figure out if the dl mgr should take the request, which looks like the part of this bug that we haven't figured out fully yet, judging from comment 3).

Comment 5

[Jason Duell]

So the major design issue here seems to be how (and where) the decision to invoke the download manager will be made.

Currently (if I understood comment 3 correctly) the docshell(s) and/or iframes that created the channel decide (presumably during OnStartRequest?) whether they will handle the channel data.  If not, the download manager is invoked.

The problem with this is that the download manager will live in the chrome process, which the docshells/iframes will be in content.  We'd like to avoid having to send OnStartRequest to the content process, only to have it decide it doesn't want to handle the channel, at which point we'd have to "rewind" the channel (which would likely have already sent some number of SendOnDataAvailable msgs to content) somehow and give it to the download manager in chrome.

JST seems to think that the logic by which the docshells decide to accept/reject handling channels is just their content-type (i.e. MIME type).  Correct?  If so, the list of content types that the content process accepts should not change much at runtime (perhaps when add-ons are added/removed/disabled), and we should be able to maintain a "subscription list": Content processes would "subscribe" to the kinds of content they're able to handle for a given request, and if no handler is subscribed, the download manager should be invoked immediately in chrome, with the child receiving only Cancel(NS_BINDING_RETARGETED).  

It's not clear to me from biesi's comment 3 how hard this would be, and/or where the refactored uriloader/nsExternalHelperService logic would live (and if in content, how and when chrome would query it). 

I think we need more input from those who know this code--bz, biesi?  We're basically stuck on the design right now.

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Currently (if I understood comment 3 correctly) the docshell(s) and/or iframes
> that created the channel decide (presumably during OnStartRequest?) whether
> they will handle the channel data.  If not, the download manager is invoked.

To a first approximation, correct.

> We'd like to avoid having to send OnStartRequest to the content process, only
> to have it decide it doesn't want to handle the channel, at which point we'd
> have to "rewind" the channel (which would likely have already sent some
> number of SendOnDataAvailable msgs to content) somehow and give it to the
> download manager in chrome.

Can we effectively move the URILoader into chrome and not have a content-side channel at all in this case?  I guess that would break too much....

> JST seems to think that the logic by which the docshells decide to
> accept/reject handling channels is just their content-type (i.e. MIME type). 

Sort of.  Arbitrary criteria can actually be involved, but not based on the _content_ process at the moment.  Chrome can have a say in what docshells in that chrome window accept.

> If so, the list of content types that the content process accepts
> should not change much at runtime (perhaps when add-ons are
> added/removed/disabled)

Yes, and plug-ins installed.

nsWebNavigationInfo::IsTypeSupportedInternal is the relevant code.

So right now I believe that uriloader theoretically assumes nothing about its consumer.  In practice the consumer is almost always a docshell.  We could change the URILoader to assume that perhaps.  Certainly on the content side.

I'd think nsExternalHelperAppService would live entirely in the chrome....  But then what happens for channels that don't run in chrome to start with (data:, for example)?

I guess the first question I have is what our desired data flow is for the cases when the channel is and is not proxied to chrome, in the cases when the data does and does not go to the original docshell that opened it.

Comment 7

[Jason Duell]

From the discussion we had this morning in the necko-dev phone conference, it sounds we'll eventually want to refactor our URILoader logic to have it live in chrome (I'll open a new bug for that), but that the easiest road to getting something that will work for now is a bit of a hack:  we will use our existing logic in content/docshell to determine if a channel should be handed to the download manager.  If so, instead of "rewinding" the channel and attaching it directly to the DL mgr in chrome, we'll simply continue sending all the data to the content process, and it will send it back to the DL manager.  Yes, this is lots of superfluous IPDL msgs.

I don't know the DL mgr code, or how it currently gets hooked up to channels, but here's a hand-wavey design:  When we realize in content that we want to send the channel's data to the DL mgr, we'l construct an IPDL actor (PDownloadManagerSink?), which will be the listener of the HttpChannelChild.  On the parent end of the constructor, we will hook up to the DL mgr somehow, and then as OnDataAvail msgs arrive in content, we will send them back via PDownloadManagerSink to chrome and the DL mgr).

Gross, but according to bz the simplest likely way to get things working quickly.  Brian, feel free to bug bz on IRC for help getting started.

Comment 8

[Jason Duell]

Added bug 564315 for the future work of moving URILoader to chrome.  Won't happen before fennec release, but maybe before e10s/Firefox

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I don't know the DL mgr code, or how it currently gets hooked up to channels,

Via DoContent on the external helper app service, and the data being pumped in there, afaik.

What would make the most sense is just proxying all that to the chrome process (probably inside the external helper app service itself).  We can send the ctor for the chrome end of stuff when the listener object for DoContent is created in the helper app service.

Comment 10

[Jason Duell]

From some discussion between Brian, sdwilsh, and myself, I think we may be close to understanding what bz is suggesting :)

So, in nsDocumentOpenInfo::DispatchContent(), when we would normally get the nsIExternalHelperAppService (EAH) and call DoContent(), we'll instead send a msg to chrome and have it do it, so the EAH logic lives in chrome.

We've got two possible ideas for how to do this:

1) We add a "createEAH" method to HttpChannelChild, and have DispatchContent set it.  Then, when mListener->OnStartRequest has completed, the HttpChannelChild can piggyback on the existing "OnStartComplete" IPDL method (that we added for content encoding support), and set a flag that tells HttpChannelParent to set up the EAH in RecvOnStartComplete.

2) We add a new IPDL protocol, "PExternalAppHandler" (PEAH), and have DispatchContent create it.  The chrome side of the protocol creates the EAH.

The reason we *might* need #2 is if we need to maintain a presence on the child side beyond the lifetime of the PHttpChannel.   My impression is that the PHttpChannel will receive OnStop and go away fairly quickly, before the download is complete.  The EAH code contains logic to do things like 1) close the window/tab if a right-click resulted in a download; and 2) cause the tab to redirect to a "Thanks for downloading Firefox!", etc. page.  We need to make sure it's got the resources to do that.

I suspect that we can use the code in bug 569227 (which gives the HttpChannelParent a pointer to the TabParent) to give the chrome EAH the ability to call PIFrameEmbedding::LoadURL.  I'm less sure about whether we can close a window using that, or need to send a message via PEAH to have the child close itself or whatever.  

There's also a potential design preference.  I think setting a flag on the channel to have it open an EAH in chrome is fine, but perhaps a separate PEAH protocol is cleaner.

The EAH winds up getting passed a "windowcontext" that gets GetInterfaced into various things--a DocumentLoader, a dialog, nsIPrompt, nsIRefreshURI, nsIDomWindow.  Hopefully we can do all the things that these do in chrome via the PIFrameEmbedding, etc.  If not, we may need the PEAH protocol, which could live in content just so that these calls could be proxied to the child, which could keep the windowcontext around.

Comment 11

[Jason Duell]

Also, we noticed today that we've got code for handing off unknown protocols to applications.  Should we file a new bug for that, or is it likely to be something we can roll into this bug?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> The reason we *might* need #2 is if we need to maintain a presence on the child

Don't you need #2 because #1 is HTTP-specific but other channels need to talk to the EAH too?

I don't follow the LoadURL bit.

Separate bug on unknown protocols, please; it's a totally different set of code operating under totally different constraints.

Comment 13

[Brian Crowder]

Attachment: WIP v1

Comment 14

[Josh Matthews [:jdm]]

Please hg add ExternalAppHelperService(Parent|Child).(cpp|h).

Comment 15

[Brian Crowder]

Attachment: With missing files (WIP still)

jdm:  Thanks for the reminder, sorry I forgot those.  They're mostly stubs right now, I'm honestly not sure how to proceed in hooking up the rest of the functionality I need, at this point.

Comment 16

[Brian Crowder]

Attachment: This has bugs, but I think all the ideas are here.

Comment 17

[Brian Crowder]

Attachment: platform pieces

This finishes up the platform pieces, I -think-.  I still have a couple of bugs:

nsHelperAppDlg.js seems to be bailing out with an error code 0x804b0002 (NS_BINDING_ABORTED).  This happens in two places, so I'm still working on where/why this is happening, though I mainly suspect http://mxr-test.konigsberg.mozilla.org/mozilla-central/source/toolkit/mozapps/downloads/nsHelperAppDlg.js#146 or thereabouts.

Clicking the same download link multiple times doesn't send me through the same path (perhaps because the data has been cached?), and I'm still trying to understand that, as well.

Comment 18

[Brian Crowder]

Attachment: working parent-side

This actually manages to invoke the dlmgr window from nsHelperAppDlg.js, but I think the child process has lost its head.  More soon.

Comment 19

[Brian Crowder]

Attachment: updated to tip

Trying this on linux forced me to update to tip, but I am still experiencing some weird bugs.  On Linux and Windows both, after the first download of a link (which seems to work perfectly, including creating the prompt to save/provide-external helper, and displaying the dl.mgr window after that has been chosen), clicking the link now no longer does anything.  On Windows, running and testing a download actually causes the parent and its debugger to freeze somehow, immediately after the confirmation button on the save dialog has been pressed.

Comment 20

[Brian Crowder]

Attachment: this is ready for review

This works and allows subsequent downloads to work.  I may have bugs to fix with respect to pages that offer downloads but ALSO redirect.  I will tackle those in follow-ups.

Comment 21

[Brian Crowder]

Attachment: Mostly passing try

This is almost entirely green on Try, with the exception of failing one uriloader/exthandler test on OS X 64.  I'm still working on this issue, but it would be nice to be able to address review comments as soon as possible, while continuing to push forward on that.

Comment 22

[Brian Crowder]

re: comment 21: my two dlmgr-like failures might've been https://bugzilla.mozilla.org/show_bug.cgi?id=488597, so I think we're good to go.

Comment 23

[Josh Matthews [:jdm]]

The test.xul change should be reverted, I believe.

>+  if (mOverrideListener) {
>+    nsresult res = mOverrideListener->OnStopRequest(aRequest, aContext, aStatusCode);
>+    mOverrideListener = nsnull;
>+  }

|res| is unused here.  Is that intentional?  If so, it would probably be better to use (void) instead.

You need a license in ExternalHelperAppServiceChild.(h|cpp), as well as ExternalHelperAppServiceParent.(h|cpp).

>+++ a/uriloader/exthandler/PExternalHelperAppService.ipdl

>+include "mozilla/net/NeckoMessageUtils.h";
>+
>+using IPC::URI;

Unneeded.

>+++ a/uriloader/exthandler/nsExternalHelperAppService.cpp
> #include "nsCRT.h"
>-
>+  

Unnecessary.

>+    mozilla::net::HttpChannelChild *httpChannel = static_cast<mozilla::net::HttpChannelChild *>(chan.get());
>+
>+    mozilla::dom::TabChild *child = static_cast<mozilla::dom::TabChild*>(tabchild.get());

Let's get some using statements in this file to avoid this mess.

>+    aStreamListener = nsnull;

Why is this here?

I notice that |ExternalHelperAppServiceChild::Recv__delete__(const bool& allow)| is not implented anywhere, and neither do you ever call Send__delete__.  Further more, the DeallocPExternalHelperAppService functions don't actually deallocate the actors.  It looks like any actors created will leak for the duration of the app.

Comment 24

[Doug Turner (:dougt)]

Comment on attachment 461308 [details] [diff] [review]
Mostly passing try


>+    manages PExternalHelperAppService;

nit:

PExternalHelperApp


>+    PExternalHelperAppService(nsCString aMimeContentType, PHttpChannel channel);
>+

comments.  what if the channel is null, what if the content type is empty.  can't you get the content type from the channel?

>+#include "mozilla/dom/PExternalHelperAppService.h"
> 
> #include "jsapi.h"
> #include "nsCOMPtr.h"
>@@ -96,9 +97,11 @@ public:
>     TabParent();
>     virtual ~TabParent();
>     void SetOwnerElement(nsIDOMElement* aElement) { mFrameElement = aElement; }
>+    nsIDOMElement* GetOwnerElement() { return mFrameElement; }
>     void SetBrowserDOMWindow(nsIBrowserDOMWindow* aBrowserDOMWindow) {
>         mBrowserDOMWindow = aBrowserDOMWindow;
>     }
>+    nsIBrowserDOMWindow *GetBrowserDOMWindow() { return mBrowserDOMWindow; }

why is GetBrowserDOMWindow needed?

>@@ -134,6 +137,11 @@ public:
>       return true;
>     }
> 
>+    virtual PExternalHelperAppServiceParent* AllocPExternalHelperAppService(
>+            const nsCString& aMimeContentType,
>+            PHttpChannelParent* channel);

align params


>+++ b/dom/ipc/test.xul
>@@ -1,7 +1,7 @@
> <?xml version="1.0"?>
> <?xml-stylesheet href="chrome://global/skin" type="text/css"?>
> <window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>-        width="800" height="800" orient="vertical" onload="initRemoteFrameScript();">
>+        width="800" height="800" orient="vertical">

clearly a bad idea. 


>-  if (aIID.Equals(NS_GET_IID(nsIAuthPromptProvider))) {
>+  if (aIID.Equals(NS_GET_IID(nsIAuthPromptProvider)) ||
>+      // URILoader needs these
>+      aIID.Equals(NS_GET_IID(nsIURIContentListener)) ||
>+      aIID.Equals(NS_GET_IID(nsILoadGroup))) {
>     if (!mTabParent)
>       return NS_NOINTERFACE;
>     return mTabParent->QueryInterface(aIID, result);

what do they really need from the load group?


> 
>-#if defined(PR_LOGGING)
>+#if defined(MOZ_LOGGING)

maybe it would be cleaner to split out the logging stuff and land it now.



>+protocol PExternalHelperAppService
>+{
>+  manager PBrowser;
>+
>+child:
>+  __delete__(bool allow);
>+};

alot of scaffolding for a new protocol.  ooc, did you consider just adding this to pnecko directly?

Comment 25

[Jason Duell]

Comment on attachment 461308 [details] [diff] [review]
Mostly passing try

Shawn taking review--may be a few days.

Comment 26

[Brian Crowder]

Attachment: without PExternalHelperApp

Thanks to previous comments and the pain involved in trying to address some of them, I've decided to make this be a one-way message for now, rather than a protocol.  This reduces the size of the patch considerably.

Comment 27

[Josh Matthews [:jdm]]

The changes look pretty good to me, however:

>nsresult res = mOverrideListener->OnStopRequest(aRequest, aContext, aStatusCode);

Something still needs to happen here - res is unused.

Comment 28

[Brian Crowder]

Attachment: review rev 2

Thanks again for the thorough review!

Comment 29

[Brian Crowder]

jdm:  Very curious to know what you think of the new "res" situation.  I'm not sure I'm happy with it, frankly.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 463006 [details] [diff] [review]
review rev 2

Note: I only reviewed the stuff in uriloader/ since that's all I'm qualified to review.  This is is going to need sr, and I suggest bz since he knows the uriloader code better than I.

For review comments with expandable code context, please see http://reviews.visophyte.org/r/463006/.

on file: uriloader/exthandler/nsExternalHelperAppService.cpp line 689
>   if (XRE_GetProcessType() == GeckoProcessType_Content) {

I would love to see more comments in this block explaining what we are
getting, and why.  I can figure it out if I stare at it for a while, but the
idea is to lower the barrier to entry on this code, not keep it the same as it
is now :)


on file: uriloader/exthandler/nsExternalHelperAppService.cpp line 717
>     HttpChannelChild *httpChannel = static_cast<HttpChannelChild *>(chan.get());
>     TabChild *child = static_cast<TabChild*>(tabchild.get());

We should be able to QI to this interfaces and not assume the static_cast is
safe.  mozilla::dom::Link does this if you need sample code.


r- because I don't see any tests for this.  We'll absolutely need them (unless you can prove to me that we already have coverage for this, but I don't think we do).

Comment 31

[Brian Crowder]

Attachment: with better comments

This patch is bound to disappoint you, sdwilsh:

1) nsIHttpChannels on the child process can only be cast to HttpChannelChild, never QI'd.  HttpChannelChild is a concrete type.  The good news is, ALL nsIHttpChannel things on the child are, in fact, HttpChannelChildren masquerading as nsHttpChannels.  This code has to stay as-is.

2) I can't produce an automated test-case for this code at this time.  There is, afaik, no infrastructure for browser-chrome tests that execute partially in the parent and partially in the child.  As far as I can tell, all mochitests run in the parent process entirely.  I think we'll have to settle for in-litmus? and an urgent follow-up bug, for now.

Comment 32

[Brian Crowder]

browser-chrome tests for Fennec are tracked in this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=489161

Comment 33

[Shawn Wilsher :sdwilsh]

Comment on attachment 463549 [details] [diff] [review]
with better comments

r=sdwilsh

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 463549 [details] [diff] [review]
with better comments

>+++ b/dom/ipc/TabParent.cpp
>+  nsIRequest *request = static_cast<nsIRequest *>(channel);

Why do you need that?  Just use |channel| as needed.

>+  nsresult rv;
>+  nsCOMPtr<nsIURILoader> uriLoader(do_GetService(NS_URI_LOADER_CONTRACTID, &rv));
>+  if (NS_FAILED(rv))
>+    return false; // uh oh!!  throw here??

Why do you need this?

>+    rv = helperAppService->DoContent(aMimeContentType,
>+                                     request,
>+                                     ir,
>+                                     PR_FALSE,
>+                                     getter_AddRefs(listener));

Hmm.  So that code will try to get the loadgroup from |ir| to remove the channel from it.  Will that work?

It will also try to do things like honor Refresh headers (which seems like it would break with this patch).  

It also looks likely that this breaks the "close the new window we just opened" stuff.

I'd like to see at least the loadgroup thing fixed; the others are _probably_ ok as followups (esp. if the third one isn't an issue for fennec), but refresh thing will definitely break some sites.

> +  pchan->OnStartRequest(request, ir);

This looks wrong.  We should just be calling OnStartRequest on |listener| directly, no?  And why passing |ir| as the context?

>+++ b/netwerk/protocol/http/HttpChannelParent.cpp
>+TabParent* HttpChannelParent::GetParent()
>+{
>+  return static_cast<TabParent *>(mTabParent.get());
>+}

This looks unused.

>+bool HttpChannelParent::OverrideChild(nsIStreamListener *listener)
>+{
>+  mOverrideListener = listener;
>+  return true;
>+}

This should be a void method... and perhaps named something sane?  I have no idea what it does by looking at the name.  Failing that, document, at least?

> HttpChannelParent::OnStartRequest(nsIRequest *aRequest, nsISupports *aContext)
> {
>+  if (mOverrideListener)
>+    return mOverrideListener->OnStartRequest(aRequest, aContext);
>+    
>   LOG(("HttpChannelParent::OnStartRequest [this=%x]\n", this));

Why do we want to skip the logging?

On the contrary, I'd think we want to log which codepath we plan to take.

That said, how can we possibly get into this situation, once we fix the OnStartRequest thing above?  It would seem better to me to assert here that mOverrideListener is null?

> HttpChannelParent::OnStopRequest(nsIRequest *aRequest, 

And here we call OnStopRequest on both mOverrideListener and via ipc?  That could use documentation....

> HttpChannelParent::OnDataAvailable(nsIRequest *aRequest, 

Again, please don't lose the logging.

It's not clear how this is all working in situations where we have to do type-sniffing.  Did we stop using a stream listener for that?  If not, aren't we either failing to pass some data to mOverrideListener or failing to type-sniff properly?

>+++ b/netwerk/protocol/http/nsHttp.h

I don't follow these changes.  PR_LOGGING is the right thing to condition logging stuff on.  Why is that being changed?

>+++ b/uriloader/base/nsURILoader.cpp
>+#ifdef MOZ_IPC
>+  if (XRE_GetProcessType() != GeckoProcessType_Content) {
>+#endif
>   NS_ASSERTION(m_targetStreamListener || NS_FAILED(rv),
>                "There is no way we should be successful at this point without a m_targetStreamListener");
>+#ifdef MOZ_IPC
>+  }
>+#endif

This looks wrong.  That assert is correct in all cases except the one where we hand the load back off to chrome, so why are we disabling it altogether?

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>-#ifdef PR_LOGGING
>+#ifdef MOZ_LOGGING

This is wrong....

>-#ifdef PR_LOGGING
>+#ifdef MOZ_LOGGING

And this.

>@@ -670,16 +680,59 @@ NS_IMETHODIMP nsExternalHelperAppService
>          isAttachment));
>     if (isAttachment)
>       reason = nsIHelperAppLauncherDialog::REASON_SERVERREQUEST;
>   }
> 
>   LOG(("HelperAppService::DoContent: mime '%s', extension '%s'\n",
>        PromiseFlatCString(aMimeContentType).get(), fileExtension.get()));
> 
>+#ifdef MOZ_IPC

Why isn'tthis at the beginning of the method?  You're going to log this request twice, looks like... (forgetting for the moment that NSPR logging in ipc land is probably all kinds of broken).

>+  if (XRE_GetProcessType() == GeckoProcessType_Content) {

It looks like this code completely breaks things like clicking on ftp:// or data:// or file:// links that lead to documents we don't handle internally in content tabs.  Why is that acceptable?

>+    NS_ASSERTION(item, "docshell tree item is null");
>+    if (!item)
>+      return NS_ERROR_FAILURE;

Why the pointless null-check?

>+    NS_ASSERTION(owner, "docshell tree owner is null");
>+    if (!owner)
>+      return NS_ERROR_FAILURE;
>+
>+    nsCOMPtr<nsITabChild> tabchild = do_GetInterface(owner);

And here?  do_GetInterface is null-safe.

>+    // Now that we have a TabChild, we need to be able to tell the
>+    // remote process which channel we're interested in.  For now,
>+    // we can only handle nsIHttpChannels.  In the future, we must
>+    // add support for FTPChannels, once those have IPDL actors.

Uh.... no.  Adding support for channels one at a time is not going to work to fix the issue above about breaking random protocols.  The right thing to do this is to have a "ship the data back and forth" codepath for the cases where we can't just handle the load all in chrome, and then create separate fast-paths like this one for http, ftp, whatever.

>+    child->SendDoExternalHelperApp(nsCString(aMimeContentType), httpChannel);

This loses the aForceSave boolean.  Why?

Comment 35

[Brian Crowder]

Attachment: the new hotness

This doesn't by any means address all of bz's comments, etc., but is a sketch of the new approach (which was the original approach jduell and I somehow talked ourselves out of): just forward everything back to the parent process and handle creating temp files, mime-service-mayhem, etc. there.  This doesn't demand any changes to how we handle encoding, etc., and shouldn't break non-HTTP channels (though right now, I think it still specializes for HTTP in a couple of spots).  More to come soon.

Comment 36

[Brian Crowder]

Attachment: the new hotness WITH missing pieces

Comment 37

[Brian Crowder]

Attachment: new hotness, now with: it actually works!

This actually works.  I was sort of expecting to have to flesh out more of this...  there is definitely going to be stuff on the child side that is broken and wrong, but that stuff should be eminently fixable.  I hope, hope, hope, that this or something close to it can be landed and other work can proceed in follow-ups?

Comment 38

[Brian Crowder]

Attachment: cleanup of new hotness

Took out printf()s and the nsURILoader.cpp changes.

Comment 39

[Brian Crowder]

Attachment: With PExternalHelperApp.ipdl

Comment 40

[Brian Crowder]

Comment on attachment 465112 [details] [diff] [review]
With PExternalHelperApp.ipdl

Forgot to re-add sr?

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 465006 [details] [diff] [review]
cleanup of new hotness

>+++ b/dom/ipc/PBrowser.ipdl

Someone who understands it needs to review the ownership model here to make sure it makes sense (e.g. the lack of __delete__ messages being sent).

>+++ b/uriloader/exthandler/ExternalHelperAppServiceChild.cpp
>+  MOZ_COUNT_CTOR(ExternalHelperAppServiceChild);

Don't need that, or the DTOR counterpart.

>+ExternalHelperAppServiceChild::OnDataAvailable(nsIRequest *request,

Need to look at the return value of mHandler->OnDataAvailable.

That said, do we need to call OnDataAvailable on mHandler at all?  Please double-check.

>+  nsresult rv;
>+  rv = input->Read(p, count, &bytesRead);

Why do you need the linebreak?

>+  data.EndWriting();
>+  if (!NS_SUCCEEDED(rv) || bytesRead != count)
>+    return rv;

This is wrong.  There's no reason bytesRead has to == count here.  This code needs to loop reading data until it gets an exception or bytesRead == 0.  See what the existing OnDataAvailable does.

>+  return NS_OK; // let the pump cancel on failure

I don't follow this comment.

This looks like the code assumes mHandler will violate the nsIStreamListener contract.  That assumption should be documented somewhere, and enforced in the callee (which it's not, that I can see).

>+ExternalHelperAppServiceChild::OnStartRequest(nsIRequest *request, nsISupports *ctx)
>+{
>+  mHandler->OnStartRequest(request, ctx);
>+  SendOnStartRequest();
>+  return NS_OK;

Why is it ok to ignore the return value from mHandler->OnStartRequest?

>+  mHandler->OnStopRequest(request, ctx, status);
>+  SendOnStopRequest(status);

Are we guaranteed that mHandler won't cancel the channel under OnStopRequest?  I guess we can assume that if you double-check it's actually true.

>+++ b/uriloader/exthandler/ExternalHelperAppServiceChild.h
>+    void SetHandler(nsIStreamListener *handler);

Please document.

>+++ b/uriloader/exthandler/ExternalHelperAppServiceParent.cpp
>+  // Set these flags to indicate that the channel has been targeted and that
>+  // we are not using the original consumer.

What flags?  Still need to set, or no?

>+
>+  AddRef(); // FIXME:  This is a hack, I think??

Er... I think so.  This is just making sure we don't die before OnStopRequest, right?  Again, check the ownership model with cjones; if this makes sense document well.

>+  helperAppService->DoContent(aMimeContentType, this, ir, PR_FALSE, getter_AddRefs(mListener));

This still worries me a tad in terms of this setting state on the chrome window.... but I think it should be ok.

This still loses that aForceSave value.  Why?

>+ExternalHelperAppServiceParent::RecvOnDataAvailable(const nsCString &data, const PRUint32 &offset, const PRUint32 &count)
>+  rv = mListener->OnDataAvailable(this, nsnull, stringStream, offset, count);

Should errors from this not propagate to the caller and cancel the channel?  Or something?  If the user cancels the transfer in the download manager, we really need to cancel the real necko channel, for example.

Followup bug?

>+  MOZ_COUNT_DTOR(ExternalHelperAppServiceParent);

Again, it's refcounted so shouldn't need this.

>+ExternalHelperAppServiceParent::GetName(nsACString &aResult)
>+{
>+  return NS_OK;

You do need a name.

>+ExternalHelperAppServiceParent::IsPending(PRBool *aResult)
>+{
>+  return NS_OK;

Uh... no.  That will make callers read random values.  Please fix.

>+ExternalHelperAppServiceParent::GetStatus(nsresult *aResult)
>+{
>+  return NS_OK;

And this.

>+ExternalHelperAppServiceParent::Cancel(nsresult aStatus)
>+{
>+  return NS_OK;

Lik I said, followup.

>+ExternalHelperAppServiceParent::Suspend()
>+{
>+  return NS_OK;

You mean NS_ERROR_NOT_IMPLEMENTED?

>+ExternalHelperAppServiceParent::Resume()
>+{
>+  return NS_OK;

And here.

>+ExternalHelperAppServiceParent::GetOriginalURI(nsIURI * *aURI)
>+{
>+  return NS_OK;

Caller just crashed reading random memory....  This needs to be fixed.

>+ExternalHelperAppServiceParent::SetOriginalURI(nsIURI *aURI)
>+{
>+  return NS_OK;

This should probably do something.

>+ExternalHelperAppServiceParent::GetURI(nsIURI **aURI)
>+{
>+  NS_ENSURE_ARG_POINTER(aURI);

Please don't null-check out params.

>+ExternalHelperAppServiceParent::Open(nsIInputStream **aResult)
>+{
>+  return NS_OK;

NS_ERROR_NOT_IMPLEMENTED, please.

>+ExternalHelperAppServiceParent::AsyncOpen(nsIStreamListener *aListener, nsISupports *aContext)
>+{
>+  return NS_OK;

And here.

>+ExternalHelperAppServiceParent::GetLoadFlags(nsLoadFlags *aLoadFlags)
>+{
>+  return NS_OK;

This should do something.  At least set the flags to 0?  Returning random values is really bad.

>+ExternalHelperAppServiceParent::SetLoadFlags(nsLoadFlags aLoadFlags)
>+{
>+  return NS_OK;

Throw?

>+ExternalHelperAppServiceParent::GetLoadGroup(nsILoadGroup* *aLoadGroup)
>+{
>+  return NS_OK;

Again, no.  Set to null?

>+ExternalHelperAppServiceParent::SetLoadGroup(nsILoadGroup* aLoadGroup)
>+{
>+  return NS_OK;

And throw?

>+ExternalHelperAppServiceParent::GetOwner(nsISupports* *aOwner)
>+{
>+  return NS_OK;

Set to null.

>+ExternalHelperAppServiceParent::SetOwner(nsISupports* aOwner)
>+{
>+  return NS_OK;

Throw?

>+ExternalHelperAppServiceParent::GetNotificationCallbacks(nsIInterfaceRequestor* *aCallbacks)

Set to null.

>+ExternalHelperAppServiceParent::SetNotificationCallbacks(nsIInterfaceRequestor* aCallbacks)

Throw.

>+ExternalHelperAppServiceParent::GetSecurityInfo(nsISupports * *aSecurityInfo)

Null?

>+ExternalHelperAppServiceParent::GetContentType(nsACString &aContentType)

Doesn't this need to work?  Maybe the exthandler doesn't use it...  Still, should make this work.  You have the type.

>+ExternalHelperAppServiceParent::SetContentType(const nsACString &aContentType)

This can probably throw.

I have to say, this setup is making the redispatch patch look like it will never happen.  :(

>+ExternalHelperAppServiceParent::GetContentCharset(nsACString &aContentCharset)

Set empty?

>+ExternalHelperAppServiceParent::SetContentCharset(const nsACString &aContentCharset)

Throw?

>+ExternalHelperAppServiceParent::GetContentLength(PRInt32 *aContentLength)

Set a non-random value.

>+ExternalHelperAppServiceParent::SetContentLength(PRInt32 aContentLength)

Throw?

What happened to implementing nsIMultiPartChannel or something?

>+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
>+#ifdef MOZ_IPC
>+#include "TabChild.h"
>+#include "nsXULAppAPI.h"
>+#include "nsPIDOMWindow.h"
>+#include "nsIDocShellTreeOwner.h"
>+#include "nsIDocShellTreeItem.h"
>+#include "mozilla/net/PHttpChannelChild.h"
>+#include "HttpChannelChild.h"
>+#include "ExternalHelperAppServiceChild.h"
>+#endif

Again, can't this block go below prlog.h?

>-#ifdef PR_LOGGING
>+#ifdef MOZ_LOGGING

And then you wouldn't need this incorrect change.

>@@ -675,6 +688,54 @@ NS_IMETHODIMP nsExternalHelperAppService
>+    nsCOMPtr<nsPIDOMWindow> window(do_GetInterface(aWindowContext));
>+    if (!window)
>+      return NS_ERROR_FAILURE;
>+    nsIDocShell *docshell = window->GetDocShell();

Why not just getInterface nsIDocShell directly?  I'd think that should work.

>+    nsCOMPtr<nsIDocShellTreeItem> item = do_QueryInterface(docshell);
>+    NS_ASSERTION(item, "docshell tree item is null");
>+    if (!item)
>+      return NS_ERROR_FAILURE;

Nix the null-check here.

>+    nsCOMPtr<nsIDocShellTreeOwner> owner;
>+    item->GetTreeOwner(getter_AddRefs(owner));
>+    NS_ASSERTION(owner, "docshell tree owner is null");
>+    if (!owner)
>+      return NS_ERROR_FAILURE;

Nix the assertion here.  And NS_ENSURE_TRUE(owner), since we do want the warning?

>+    nsCOMPtr<nsITabChild> tabchild = do_GetInterface(owner);

Shouldn't that be QI?

>+    nsCOMPtr<nsIHttpChannel> chan = do_QueryInterface(aRequest);
>+    if (!chan)
>+      return NS_ERROR_NOT_IMPLEMENTED;

Why do you need that block?  I don't think you want to.

>+    mozilla::dom::TabChild *child = static_cast<mozilla::dom::TabChild*>(tabchild.get());

This needs some |namespace dom = mozilla::dom| love.

>+    pc = child->SendPExternalHelperAppServiceConstructor(IPC::URI(uri), nsCString(aMimeContentType));

Watch your 80th column?

>+    mozilla::dom::ExternalHelperAppServiceChild *childListener = static_cast<mozilla::dom::ExternalHelperAppServiceChild *>(pc);

And here.

>+    nsExternalAppHandler * handler = new nsExternalAppHandler(nsnull,

Are you sure it can handle a null mimeinfo?  Audit?  It certainly dereferences it without checking a bunch of places...

Comment 42

[Josh Matthews [:jdm]]

bz is right about the lifetime management concerns.  There are three distinct problems:

1) the __delete__ message is not even implemented
2) the __delete__ message is not called, so the protocols will leak
3) the explicit Addref and Release calls should occur in the corresponding AllocPFoo/DeallocPFoo functions, with PFoo::Send__delete__(this) being called in OnStopRequest instead.

Comment 43

[Brian Crowder]

Attachment: With bz's review comments

I have some other comments from previous reviews to re-apply here and some more local testing to do.  Will follow-up tomorrow morning.

Comment 44

[Brian Crowder]

Attachment: lots of cleanup, better resource management

Comment 45

[Brian Crowder]

Attachment: lots of cleanup, better resource management

Last attachment was a bit shy of fresh, this is better.

Comment 46

[Brian Crowder]

I will implement nsIMultipartChannel in a follow-up bug, btw.

Comment 47

[Brian Crowder]

Attachment: more tweaks

Comment 48

[Brian Crowder]

Attachment: another swing, thanks to dougt review on irc

Comment 49

[Doug Turner (:dougt)]

Comment on attachment 465875 [details] [diff] [review]
more tweaks


>+    PExternalHelperApp(URI uri, nsCString aMimeContentType, bool aForceSave, PRInt32 aContentLength);


aContentLength should be unsigned, right?  or does -1 mean something.

>+    virtual PExternalHelperAppChild *AllocPExternalHelperApp(
>+            const IPC::URI &uri,
>+            const nsCString& aMimeContentType,
>+            const bool& aForceSave,
>+            const PRInt32& aContentLength);

indentation



>+    virtual PExternalHelperAppParent* AllocPExternalHelperApp(
>+            const IPC::URI& uri,
>+            const nsCString& aMimeContentType,
>+            const bool& aForceSave,
>+            const PRInt32& aContentType);
>+    virtual bool DeallocPExternalHelperApp(PExternalHelperAppParent* aService);

indentation


>-
>+#warning Undefining PR_LOGGING!!

really?


>+ExternalHelperAppChild::ExternalHelperAppChild()
>+{
>+  printf("%s\n", __PRETTY_FUNCTION__);
>+}
>+
>+ExternalHelperAppChild::~ExternalHelperAppChild()
>+{
>+  printf("%s\n", __PRETTY_FUNCTION__);
>+}

printfs throughout file


>+ExternalHelperAppChild::OnDataAvailable(nsIRequest *request,
>+                                               nsISupports *ctx,
>+                                               nsIInputStream *input,
>+                                               PRUint32 offset,
>+                                               PRUint32 count)

indentation throughout file

>+  // FIXME: Eventually we should implement this:
>+  // mHandler->OnStartRequest(request, ctx);
>+  SendOnStartRequest();
>+  return NS_OK;

are you going to file a follow up bug?


>new file mode 100644

license block needed on all new files.



>+namespace dom {
>+class ExternalHelperAppChild :
>+    public PExternalHelperAppChild
>+  , public nsIStreamListener

i usually see class declarations like:

class Name: public nsIA,
            public nsIB


>+void
>+ExternalHelperAppParent::Init(TabParent *parent,
>+                              const nsCString &aMimeContentType,
>+                              const PRBool& aForceSave)
>+{

test for a non-null parent here, and that frame->GetOwnerDoc() is non-null?

>+  nsresult rv;
>+  nsCOMPtr<nsIExternalHelperAppService> helperAppService =
>+    do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
>+  NS_ASSERTION(helperAppService, "No Helper App Service!");

drop the nsresult.

>+  mPending = PR_FALSE;
>+  mListener->OnStopRequest(this, nsnull, code);
>+  unused << Send__delete__(this);

lots of questions about Send__delete__() on irc.

Do we need to also have a Recv__delete__ implementation?





>+ExternalHelperAppParent::GetOriginalURI(nsIURI * *aURI)

NS_ASSERTIONs for null parameter

>+{
>+  *aURI = mURI;

can mURI be null (maybe if we can't deserialize?

>+  NS_ADDREF(*aURI);

NS_IF_ADDREF(*aURI = mURI);

=

>+protocol PExternalHelperApp
>+{
>+  manager PBrowser;
>+
>+parent:
>+  OnStartRequest();
>+  OnDataAvailable(nsCString data, PRUint32 offset, PRUint32 count);
>+  OnStopRequest(nsresult code);
>+
>+child:
>+  __delete__();
>+};

This feels really low level.. jduell, do you have anything similar that brian can pinch or is adding a new protocol like this fine for now.


> 
>@@ -147,7 +160,9 @@ PRLogModuleInfo* nsExternalHelperAppServ
> // Using level 3 here because the OSHelperAppServices use a log level
> // of PR_LOG_DEBUG (4), and we want less detailed output here
> // Using 3 instead of PR_LOG_WARN because we don't output warnings
>+#undef LOG
> #define LOG(args) PR_LOG(mLog, 3, args)
>+#undef LOG_ENABLED
> #define LOG_ENABLED() PR_LOG_TEST(mLog, 3)

remove please


Also, i think everyone would love to see unit tests.  you know unit tests are the bees knees.

Comment 50

[Josh Matthews [:jdm]]

In response to the various questions I missed on IRC:

* there is no need to provide a Recv__delete__ implementation unless there is a specific action that should occur on actor deletion that is better done outside of the DeallocPFoo function.
* The Send__delete__ call is always necessary for protocol actors or they never die - Send__delete__ triggers Recv__delete__ -> ActorDestroy -> DeallocPFoo
* The lifetime management changes here look good to me.

Comment 51

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 465926 [details] [diff] [review]
another swing, thanks to dougt review on irc

+++ b/nsprpub/pr/include/prlog.h
+#warning Undefining PR_LOGGING!!

I don't think you want to check this in?

+++ b/uriloader/exthandler/ExternalHelperAppChild.cpp

need a license header

+ExternalHelperAppChild::OnDataAvailable(nsIRequest *request,
+                                               nsISupports *ctx,
+                                               nsIInputStream *input,
+                                               PRUint32 offset,
+                                               PRUint32 count)

incorrect indentation

+  // FIXME: Error handling?

indeed, please fix? :)

+  data.EndWriting();

No need for this

+  if (!NS_SUCCEEDED(rv) || bytesRead != count)

!NS_SUCCEEDED -> NS_FAILED

+++ b/uriloader/exthandler/ExternalHelperAppChild.h

also needs a license

+namespace dom {
+class ExternalHelperAppChild :

please add an empty line in between

+++ b/uriloader/exthandler/ExternalHelperAppParent.cpp

license

+ExternalHelperAppParent::RecvOnStartRequest()
+{
+  mPending = PR_TRUE;
+  mListener->OnStartRequest(this, nsnull);

Hmm, it does look like you can get away with not checking rv here because OnStartRequest will never return an error, but ideally you should check that (and not send further OnDataAvailable calls when that happens)

+  rv = mListener->OnDataAvailable(this, nsnull, stringStream, offset, count);

This one does need to handle an error return (same way as OnStartRequest)

+ExternalHelperAppParent::GetStatus(nsresult *aResult)
+{
+  *aResult = NS_OK;

That's incorrect, though maybe not critical...

+ExternalHelperAppParent::Cancel(nsresult aStatus)
+{
+  // FIXME: Canceling the parent should actually do stuff to the child

You can't leave this unimplemented, this will lead to wrong behaviour in some cases, possibly crashes. When Cancel gets called, you shouldn't call OnDataAvailable on the listener.

+ExternalHelperAppParent::SetOriginalURI(nsIURI *aURI)
+{
+  return NS_OK;

I think it would be better to return NOT_IMPLEMENTED here

+ExternalHelperAppParent::GetNotificationCallbacks(nsIInterfaceRequestor* *aCallbacks)
+{
+  aCallbacks = nsnull;

Should be *aCallbacks.

+  aContentType = "";

Usually aContentType.Truncate() is used

+  aContentCharset = "";

same

As a sidenote, this will break downloads > 4 GB (or at least, progress reporting for them). See the GetPropertyAsInt64(NS_CHANNEL_PROP_CONTENT_LENGTH) code in nsExternalHelperAppService.cpp.

+++ b/uriloader/exthandler/ExternalHelperAppParent.h

license

+++ b/uriloader/exthandler/Makefile.in
+            -I$(topsrcdir)/content/events/src \

What's that for?

+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
 #include "nsCRT.h"
-
+  
 #include "nsLocalHandlerApp.h"

Please don't add trailing whitespace

+    channel->GetContentLength(&contentLength);

As mentioned above... you need to deal with 64-bit lengths.

+    typedef mozilla::dom::TabChild TabChild;

using mozilla::dom::TabChild; ?

+    typedef mozilla::dom::ExternalHelperAppChild ExternalHelperAppChild;

same

+    // FIXME:  Eventually we'll use this original listener to finish up client-side
+    // work, such as handling redirects and/or closing a no-longer-needed window.

redirects have already happened at this point, fwiw.

Comment 52

[Christian :Biesinger (don't email me, ping me on IRC)]

Another note: what's the plan for making "save link target as" etc. work? Or, are context menus already handled by the parent?

If they aren't, they currently go through nsIWebProgressListener in some cases - see http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#943 and http://mxr.mozilla.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1027 (and contentAreaUtils.js)

Comment 53

[Brian Crowder]

dougt's review:

(In reply to comment #49)
> aContentLength should be unsigned, right?  or does -1 mean something.

-1 is "we don't know yet"


> >+    virtual PExternalHelperAppChild *AllocPExternalHelperApp(
> >+            const IPC::URI &uri,
> >+            const nsCString& aMimeContentType,
> >+            const bool& aForceSave,
> >+            const PRInt32& aContentLength);
> 
> indentation
> 
> 
> 
> >+    virtual PExternalHelperAppParent* AllocPExternalHelperApp(
> >+            const IPC::URI& uri,
> >+            const nsCString& aMimeContentType,
> >+            const bool& aForceSave,
> >+            const PRInt32& aContentType);
> >+    virtual bool DeallocPExternalHelperApp(PExternalHelperAppParent* aService);
> 

My indentation here matches other spots in this file.


> >+  // FIXME: Eventually we should implement this:
> are you going to file a follow up bug?

Yes.


> >+namespace dom {
> >+class ExternalHelperAppChild :
> >+    public PExternalHelperAppChild
> >+  , public nsIStreamListener
> 
> i usually see class declarations like:

Not in the IPDL code; see TabChild.cpp TabChild::TabChild for the template I tried to follow.


> 
> >+void
> >+ExternalHelperAppParent::Init(TabParent *parent,
> >+                              const nsCString &aMimeContentType,
> >+                              const PRBool& aForceSave)
> >+{
> 
> test for a non-null parent here, and that frame->GetOwnerDoc() is non-null?

I'll be happy to put assertions for these, not sure if a release/runtime test is needed?  Can someone say so definitively?


> >+  nsresult rv;
> >+  nsCOMPtr<nsIExternalHelperAppService> helperAppService =
> >+    do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
> >+  NS_ASSERTION(helperAppService, "No Helper App Service!");
> 
> drop the nsresult.

I need it for do_GetService...  or can I just pass a NULL there?


> This feels really low level.. jduell, do you have anything similar that brian
> can pinch or is adding a new protocol like this fine for now.

I don't think there exists any such thing yet as an nsIIPCForwardedStreamListener or anything along those lines.  I -do- think something like that would make this (and probably a lot of future code) cleaner.  That said, I don't think we can afford to wait for it, either.


> 
> Also, i think everyone would love to see unit tests.  you know unit tests are
> the bees knees.

adifire (GSoC helper on IRC) is working on porting existing unit tests to IPC-land.



biesi's review:


> +  // FIXME: Error handling?
> 
> indeed, please fix? :)

Actually, I think the error handling here is sufficient (we'll propogate errors to the code that is feeding this listener and it should Do the Right Thing.  No?


> +ExternalHelperAppParent::Cancel(nsresult aStatus)
> +{
> +  // FIXME: Canceling the parent should actually do stuff to the child
> 
> You can't leave this unimplemented, this will lead to wrong behaviour in some
> cases, possibly crashes. When Cancel gets called, you shouldn't call
> OnDataAvailable on the listener.
> 

Okay, I took a stab at this.  More feedback would be nice.


> As a sidenote, this will break downloads > 4 GB (or at least, progress
> reporting for them). See the GetPropertyAsInt64(NS_CHANNEL_PROP_CONTENT_LENGTH)
> code in nsExternalHelperAppService.cpp.

Ugh.


> +++ b/uriloader/exthandler/Makefile.in
> +            -I$(topsrcdir)/content/events/src \
> 
> What's that for?

TabChild.h includes nsDOMEventTargetHelper.h (indirectly?).  Fun, huh?


> +    channel->GetContentLength(&contentLength);
> 
> As mentioned above... you need to deal with 64-bit lengths.
> 

I want to get what I have up, next rev will have 64-bit lengths....

Comment 54

[Brian Crowder]

Attachment: Addressing biesi and dougt's comments, except for 64-bit issue

Comment 55

[Brian Crowder]

Comment on attachment 466496 [details] [diff] [review]
Addressing biesi and dougt's comments, except for 64-bit issue

Making dwittoccio a real reviewer!

Comment 56

[Brendan Eich [:brendan]]

> My indentation here matches other spots in this file.

roc and others (not everyone!) argued for uniform style and made me call some compromise shots. So we don't defer to file-local style if the C++ style guide addresses the issue.

/be

Comment 57

[Brian Crowder]

Attachment: adds 64-bit support via propertybag and fixes a UMR bug in previous

Comment 58

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #53)
> > >+  nsresult rv;
> > >+  nsCOMPtr<nsIExternalHelperAppService> helperAppService =
> > >+    do_GetService(NS_EXTERNALHELPERAPPSERVICE_CONTRACTID, &rv);
> > >+  NS_ASSERTION(helperAppService, "No Helper App Service!");
> > 
> > drop the nsresult.
> 
> I need it for do_GetService...  or can I just pass a NULL there?

That parameter is optional, you don't need to pass anything :-)

> > +  // FIXME: Error handling?
> > 
> > indeed, please fix? :)
> 
> Actually, I think the error handling here is sufficient (we'll propogate errors
> to the code that is feeding this listener and it should Do the Right Thing. 
> No?

SetLength can fail, right? Or is that no longer true? Historically when SetLength fails, you get a zero-length string (a constant buffer), so you'd end up writing to random memory locations, which is not so good.


Going to look at the patch now...

Comment 59

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 466678 [details] [diff] [review]
adds 64-bit support via propertybag and fixes a UMR bug in previous

+++ b/uriloader/exthandler/ExternalHelperAppChild.cpp
+  if (mCanceled)
+    request->Cancel(mStatus);

Two notes:
- I think you should replace the request->Cancel line with "return mStatus;". That way you don't call SendOnDataAvailable for a cancelled channel. And returning an error has the side-effect of cancelling the channel.
- You shouldn't need both mCanceled and mStatus - NS_FAILED(mStatus) should be enough to indicate that the channel was cancelled. Cancelling with a success code is not supported.

+  rv = input->Read(p, count, &bytesRead);

BTW, you could avoid copying the data by using ReadSegments instead of Read, but that may not be worth it (would add some complexity)

+++ b/uriloader/exthandler/ExternalHelperAppChild.h
+class ExternalHelperAppChild :
+    public PExternalHelperAppChild
+  , public nsIStreamListener

Shouldn't the colon be on the next line, as in the initializer list in the .cpp file?

+    bool RecvCancel(const nsresult &aStatus);

virtual? (this is inherited from PExternalHelperAppChild, right?)


As a sidenote, remember that you'll have to implement other channel interfaces later (nsIHttpChannel, etc)

+++ b/uriloader/exthandler/ExternalHelperAppParent.cpp
+ExternalHelperAppParent::RecvOnDataAvailable(const nsCString &data, const PRUint32 &offset, const PRUint32 &count)
+{
+
+  nsCOMPtr<nsIInputStream> stringStream;

please remove that empty line

+  if (!mCanceled && mStatus == NS_OK)
+    mStatus = mListener->OnDataAvailable(this, nsnull, stringStream, offset, count);

as an optimization, you could check that before creating the input stream

(shouldn't you check NS_SUCCEEDED instead of ==NS_OK? That said, the same cancelled/NS_FAILED(status) comment applies)

+ExternalHelperAppParent::RecvOnStopRequest(const nsresult &code)
+{
+  mPending = PR_FALSE;
+  mListener->OnStopRequest(this, nsnull, code);

Here's an edge case:

Child sends onDataAvailable
Child sends onStopRequest
Parent receives onDataAvailable, calls Cancel
Parent receives onStopRequest

In that case, mListener should see a failure status in OnStopRequest. So I think what you need is:

  if (NS_SUCCEEDED(code) && NS_FAILED(mStatus))
    code = mStatus;


+++ b/uriloader/exthandler/nsExternalHelperAppService.cpp
+    
+    PRInt32 clen;
+    channel->GetContentLength(&clen);
+

I don't think you need this anymore

+    //                                                           nsCAutoString(""),

EmptyCString()

+  mContentLength.mValue = GetContentLengthAsInt64(request);

mContentLength should really become a PRInt64. If you want, feel free to do that change in this patch :-)

Comment 60

[Christian :Biesinger (don't email me, ping me on IRC)]

BTW, why did you remove bug 567267 from the dependency list? That's still necessary for correct downloads of certain .tar.gz files, right?

Comment 61

[Brian Crowder]

No, the decoding should still work fine on the parent side without shifting nsIEncodedChannel around...  I think this means that we're doing decoding in the child process, though?

Comment 62

[Brian Crowder]

Attachment: addressing bz's sr+ comments and fixing cancellation (needed nsICancelable)

Comment 63

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #61)
> No, the decoding should still work fine on the parent side without shifting
> nsIEncodedChannel around...  I think this means that we're doing decoding in
> the child process, though?

Aren't you calling SetApplyConversion way too late?

Comment 64

[Brian Crowder]

Attachment: Don't need nsICancelable

Still carrying fwd biesi's sr+, afaik

Comment 65

[Brian Crowder]

I will fix issues with SetApplyConversion/nsIEncodedChannel in a follow-up

Comment 66

[Brian Crowder]

Attachment: whitespace fixes

This brings me closer to the style guidelines for whitespace, but I'm still leary of munging the style in [TabChild|TabParent].[cpp|h], which are not my modules.

Comment 67

[[e-mail]]

Comment on attachment 466822 [details] [diff] [review]
whitespace fixes

>diff --git a/uriloader/exthandler/ExternalHelperAppChild.cpp b/uriloader/exthandler/ExternalHelperAppChild.cpp

>+ * The Original Code is the Mozilla browser.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Netscape Communications, Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 1999
>+ * the Initial Developer. All Rights Reserved.

Needs updating -- use boilerplate from http://www.mozilla.org/MPL/boilerplate-1.1/ for all new files please

>+void
>+ExternalHelperAppChild::SetHandler(nsIStreamListener *handler)
>+{
>+  mHandler = handler;
>+}

Make this inline in the .h?

>+NS_IMETHODIMP
>+ExternalHelperAppChild::OnDataAvailable(nsIRequest *request,
>+                                        nsISupports *ctx,
>+                                        nsIInputStream *input,
>+                                        PRUint32 offset,
>+                                        PRUint32 count)
>+{

>+  nsCString data;
>+  data.SetLength(count);
>+  if (data.Length() != count)
>+    return NS_ERROR_OUT_OF_MEMORY;
>+
>+  char *p = data.BeginWriting();
>+  PRUint32 bytesRead;
>+  nsresult rv;
>+  rv = input->Read(p, count, &bytesRead);
>+  if (NS_FAILED(rv) || bytesRead != count)
>+    return rv;

Instead of all this just use:

  nsCString data;
  nsresult rv = NS_ReadInputStreamToString(input, data, count);
  if (NS_FAILED(rv))
    return rv;

>diff --git a/uriloader/exthandler/ExternalHelperAppParent.cpp b/uriloader/exthandler/ExternalHelperAppParent.cpp

>+bool
>+ExternalHelperAppParent::RecvOnDataAvailable(const nsCString& data,
>+                                             const PRUint32& offset,
>+                                             const PRUint32& count)
>+{

Assert mPending here?

>+  if (NS_FAILED(mStatus))
>+    return true;
>+
>+  nsCOMPtr<nsIInputStream> stringStream;
>+  nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream), data.get(), count, NS_ASSIGNMENT_DEPEND);
>+  if (NS_FAILED(rv))
>+    return false;

Terminating the child is pretty serious, but I guess that's OK since NS_NewByteInputStream with NS_ASSIGNMENT_DEPEND should never fail. Maybe you just want to assert that it succeeded, though.

>+bool
>+ExternalHelperAppParent::RecvOnStopRequest(const nsresult& code)
>+{

Assert mPending here?

>+  mPending = PR_FALSE;
>+  mListener->OnStopRequest(this, nsnull,
>+                           (NS_SUCCEEDED(code) && NS_FAILED(mStatus)) ? mStatus : code);
>+  unused << Send__delete__(this);

Classy. ;)

>+NS_IMETHODIMP
>+ExternalHelperAppParent::GetOriginalURI(nsIURI * *aURI)
>+{
>+  NS_ASSERTION(aURI, "null aURI parameter!");

No need to assert; you'll crash on the very next line.

>+  NS_IF_ADDREF(*aURI = mURI);
>+  return NS_OK;
>+}

>+NS_IMETHODIMP
>+ExternalHelperAppParent::GetURI(nsIURI **aURI)
>+{
>+  *aURI = mURI;
>+  NS_ADDREF(*aURI);

NS_IF_ADDREF(*aURI = mURI);

You used NS_IF_ADDREF in ::GetOriginalURI(), so either it can be null or not, in which case either both or neither of your addrefs should be the IF variety. ;)

>+NS_IMETHODIMP
>+ExternalHelperAppParent::GetContentLength(PRInt32 *aContentLength)
>+{
>+  if (mContentLength > PR_INT32_MAX || mContentLength < 0)
>+    *aContentLength = -1;

Hmm. Probably want to throw here? Or did you have a reason for returning -1? (If -1 is the convention for "I don't know" then I suppose that sounds OK.)

>diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp

> NS_IMETHODIMP nsExternalHelperAppService::DoContent(const nsACString& aMimeContentType,
>                                                     nsIRequest *aRequest,
>                                                     nsIInterfaceRequestor *aWindowContext,
>                                                     PRBool aForceSave,
>                                                     nsIStreamListener ** aStreamListener)
> {
>   nsAutoString fileName;
>   nsCAutoString fileExtension;
>   PRUint32 reason = nsIHelperAppLauncherDialog::REASON_CANTHANDLE;
>   nsresult rv;
> 
>   // Get the file extension and name that we will need later
>   nsCOMPtr<nsIChannel> channel = do_QueryInterface(aRequest);
>-  if (channel) {
>+  nsCOMPtr<nsIURI> uri;
>+  
>+#ifdef MOZ_IPC

>+  if (channel)
>+    channel->GetURI(getter_AddRefs(uri));

This is done later on (in the !MOZ_IPC block), so just pull it out front?

>+    // FIXME:  Eventually we'll use this original listener to finish up client-side
>+    // work, such as closing a no-longer-needed window.

File a bug and quote # here?

>+    // nsExternalAppHandler * handler = new nsExternalAppHandler(nsnull,
>+    //                                                           EmptyCString(),
>+    //                                                           aWindowContext,
>+    //                                                           fileName,
>+    //                                                           reason,
>+    //                                                           aForceSave);
>+    // if (!handler)
>+    //   return NS_ERROR_OUT_OF_MEMORY;
>+    //
>+    // childListener->SetHandler(handler);
>+
>+    return NS_OK;
>+  }
>+#endif // MOZ_IPC
>+
>+if (channel) {

Needs indenting.

r=dwitte with fixes.

Comment 68

[Brian Crowder]

Attachment: the one to land

This addresses everything mentioned previously and is therefore totally perfect and bug-free.  Except for the follow-up bugs and mistakes I've made that no one has caught yet.  :)

Comment 69

[Brian Crowder]

Attachment: missed one tiny thing. This is the one to land

Comment 70

[Brian Crowder]

http://hg.mozilla.org/mozilla-central/rev/cac7482b3cd8