Markus Stange [:mstange] Reporter
	Description • 7 years ago
regression-window str

[Tracking Requested - why for this release]: regression

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:55.0) Gecko/20100101 Firefox/55.0
20170322030208

Steps to reproduce:
 1. Go to https://www.google.com/chrome/
 2. Wait for the animation to finish.
 3. Click "Download now".
 4. Click "Accept and Install".
 5. In the download dialog, select "Save File".
 6. Click OK.

Expected results:
You should download a file called googlechrome.dmg. It should show up in the downloads panel and on your hard drive.

Actual results:
The download disappears.
You get redirected to https://www.google.com/chrome/browser/thankyou.html?platform=mac .
The .part file that appeared in your Downloads folder while the Downloads dialog was open vanishes.
The downloads panel is empty.

mozregression found this range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34585620e529614c79ecc007705646de748e592d&tochange=fa55f8f56a1d5e64c3682b500127786082354691

Suspects are bug 1301056 and bug 1334693.

	Marcia Knous [:marcia]
	Comment 1 • 7 years ago

Tracking 55+ for this regression.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 2 • 7 years ago

The new window opened by the chrome download is a window instead of a tab. My testing only opened new tabs, so the window context stayed the same. I have a patch to fix this. I'll see what I can do about writing a test for this tomorrow.

	Mike Conley (:mconley) (:⚙️)
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

https://reviewboard.mozilla.org/r/123302/#review126564

I think this makes good sense, but I have a few questions (see below). Also, we should get that test you were talking about in here or in another commit in this series. :)

::: uriloader/exthandler/ExternalHelperAppChild.cpp:67
(Diff revision 1)
>  ExternalHelperAppChild::OnStartRequest(nsIRequest *request, nsISupports *ctx)
>  {
>    nsresult rv = mHandler->OnStartRequest(request, ctx);
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_UNEXPECTED);
>  
> +

Nit - needless newline.

::: uriloader/exthandler/ExternalHelperAppChild.cpp:74
(Diff revision 1)
> +  // loaded for. In that case, the TabParent in the parent context might then
> +  // point to the wrong window. Re-send the window context along with either
> +  // DivertToParent or SendOnStartRequest just in case.
> +  nsCOMPtr<nsPIDOMWindowOuter> window =
> +    do_GetInterface(mHandler->GetDialogParent());
> +  TabChild* tabChild = window ? mozilla::dom::TabChild::GetFrom(window) : nullptr;

Just so I'm clear on this, if we're in non-e10s mode, `GetFrom(window)` will return us nullptr, right?

Also, are we certain this doesn't need to be `RefPtr`? Do we know this TabChild is going to stay alive as long as we're touching it (and passing it along) here?

::: uriloader/exthandler/ExternalHelperAppChild.cpp:108
(Diff revision 1)
>  }
>  
>  nsresult
>  ExternalHelperAppChild::DivertToParent(nsIDivertableChannel *divertable,
> -                                       nsIRequest *request)
> +                                       nsIRequest *request,
> +                                       TabChild* aBrowser)

`nsIRequest *request` vs `TabChild* aBrowser`. While I prefer the `Type* name` style, I think consistency trumps, and we should stick with `Type *name`, unless you want to fix the rest of this file (maybe in a follow-up?).

::: uriloader/exthandler/ExternalHelperAppParent.cpp:76
(Diff revision 1)
> +}
> +
> +void
> +UpdateContentContext(nsIStreamListener* aListener, PBrowserParent* aBrowser)
> +{
> +  nsCOMPtr<nsIInterfaceRequestor> window = GetWindowFromTabParent(aBrowser);

We should probably MOZ_ASSERT aListener and aBrowser here. Are there cases where they could be nullptr? Should we return early if they are?

::: uriloader/exthandler/ExternalHelperAppParent.cpp:146
(Diff revision 1)
>  
>  mozilla::ipc::IPCResult
> -ExternalHelperAppParent::RecvOnStartRequest(const nsCString& entityID)
> +ExternalHelperAppParent::RecvOnStartRequest(const nsCString& entityID,
> +                                            PBrowserParent* contentContext)
>  {
>    MOZ_ASSERT(!mDiverted, "child forwarding callbacks after request was diverted");

Maybe MOZ_ASSERT contentContext?

::: uriloader/exthandler/ExternalHelperAppParent.cpp:191
(Diff revision 1)
>  
>  mozilla::ipc::IPCResult
> -ExternalHelperAppParent::RecvDivertToParentUsing(PChannelDiverterParent* diverter)
> +ExternalHelperAppParent::RecvDivertToParentUsing(PChannelDiverterParent* diverter,
> +                                                 PBrowserParent* contentContext)
>  {
>    MOZ_ASSERT(diverter);

Maybe MOZ_ASSERT contentContext here too?

::: uriloader/exthandler/nsExternalHelperAppService.cpp:843
(Diff revision 1)
> +  // NB: ExternalHelperAppParent depends on this listener always being an
> +  // nsExternalAppHandler. If this changes, make sure to update that code.

Correct me if I'm wrong, but we'll just straight-up not compile if that changes, right?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 7 years ago
mozreview-review-reply

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

https://reviewboard.mozilla.org/r/123302/#review126564

> Just so I'm clear on this, if we're in non-e10s mode, `GetFrom(window)` will return us nullptr, right?
> 
> Also, are we certain this doesn't need to be `RefPtr`? Do we know this TabChild is going to stay alive as long as we're touching it (and passing it along) here?

In non-e10s mode, none of this code runs. The `ExternalHelperAppChild/Parent` code can only be instantiated by the child process. In non-e10s, we use `nsExternalHelperAppService` directly.

`window` in this case is the parent window (that is, the one that we're not closing). The window and docshell hold the tab child alive long enough for our uses here without needing more reference counting.

> We should probably MOZ_ASSERT aListener and aBrowser here. Are there cases where they could be nullptr? Should we return early if they are?

I actually don't know if we can assert that `aBrowser` is non-null. The existing code null-checks it. I've added an assertion that `aListener` is non-null, though.

> Maybe MOZ_ASSERT contentContext?

I'm not confident enough to do this for the moment.

> Maybe MOZ_ASSERT contentContext here too?

Ditto here.

> Correct me if I'm wrong, but we'll just straight-up not compile if that changes, right?

Not necessarily. `mListener` is of type `nsCOMPtr<nsIStreamListener>`, which could be any number of classes. We're assuming here that `nsExternalHelperAppService::DoContent` only ever creates `nsExternalAppHandler`, but nothing in the type system enforces that. If we ever create another stream listener for `nsExternalHelperAppService`, it'll still cast fine to `nsIStreamListener` and the `static_cast` here will happily do the wrong thing.

Thanks, C++!

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 7 years ago

I haven't had time yet to look into writing a test for this. I'll try to get to it soon. The trick, I think, will be shimming the "@mozilla.org/helperapplauncherdialog;1" contract to do the test and make sure the right parent window is passed in.

	Mike Conley (:mconley) (:⚙️)
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

https://reviewboard.mozilla.org/r/123302/#review128886

This seems sane to me, but I really really really would also love to see a test for this. Thanks!

::: uriloader/exthandler/ExternalHelperAppChild.h:40
(Diff revision 2)
>      virtual mozilla::ipc::IPCResult RecvCancel(const nsresult& aStatus) override;
>  private:
>      virtual ~ExternalHelperAppChild();
> -    MOZ_MUST_USE nsresult DivertToParent(nsIDivertableChannel *divertable, nsIRequest *request);
> +    MOZ_MUST_USE nsresult DivertToParent(nsIDivertableChannel *divertable,
> +                                         nsIRequest *request,
> +                                         TabChild* browser);

browser -> tabChild (see below)

::: uriloader/exthandler/ExternalHelperAppChild.cpp:106
(Diff revision 2)
> -                                       nsIRequest *request)
> +                                       nsIRequest *request,
> +                                       TabChild* browser)

Same comment as before:

> `nsIRequest *request` vs `TabChild* aBrowser`.
> While I prefer the `Type* name` style, I think
> consistency trumps, and we should stick with
> `Type *name`,  unless you want to fix the rest
> of this file (maybe in a follow-up?)."

::: uriloader/exthandler/ExternalHelperAppChild.cpp:107
(Diff revision 2)
>  }
>  
>  nsresult
>  ExternalHelperAppChild::DivertToParent(nsIDivertableChannel *divertable,
> -                                       nsIRequest *request)
> +                                       nsIRequest *request,
> +                                       TabChild* browser)

I also think it's probably more internally consistent to call this "tabChild" instead of "browser".

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

https://reviewboard.mozilla.org/r/123302/#review129150

	Pulsebot
	Comment 11 • 7 years ago

Pushed by mrbkap@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/787274149cb6
Update the content context if we close the downloading window. r=mconley

	Carsten Book [:Tomcat]
	Comment 12 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/787274149cb6

	Almas Hossain[:mAlmas]
	Comment 13 • 7 years ago

I have reproduce this bug with Nightly 55.0a1 (2017-03-23) (64- bit) in Windows 10.

This bug's fix is verified with latest Nightly 55.0a1.
 
Build ID   :	20170426030329
User Agent :	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

[bugday-20170426]

	Gerry Chang [:gchang]
	Comment 14 • 7 years ago

Hi :mrbkap, 
Per comment #21 in bug 1301056, can you create an uplift request for Beta54?

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 15 • 7 years ago

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1301056
[User impact if declined]: some downloads will fail
[Is this code covered by automated tests?]: yes (see bug 1359651)
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's tested now!
[String changes made/needed]:

	Gerry Chang [:gchang]
	Comment 16 • 7 years ago

Comment on attachment 8850782 [details]
Bug 1350058 - Update the content context if we close the downloading window.

Required by bug 1301056 and was verified. Beta54+. Should be in 54 beta 4.

	Ryan VanderMeulen [:RyanVM]
	Comment 17 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/8cee26a9755f

	Andrei Vaida [:avaida]
	Comment 18 • 7 years ago

(In reply to Blake Kaplan (:mrbkap) from comment #15)
> [Is this code covered by automated tests?]: yes (see bug 1359651)
> [Has the fix been verified in Nightly?]: Yes.
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Blake's assessment on manual testing needs and the fact that this fix has automated coverage.

	Fahima Zulfath
	Comment 19 • 7 years ago

The issue is no longer reproducible on Firefox Nightly 55.0a1 (build ID: 20170503030212). Could see the chrome setup in downloads.
Test were performed under Windows 10x64.
[bugday-20170503]