Closed Bug 1350058 Opened 3 years ago Closed 3 years ago

Can no longer download Chrome

Categories

(Firefox :: File Handling, defect)

55 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox52 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- fixed
firefox55 + verified

People

(Reporter: mstange, Assigned: mrbkap)

References

Details

(Keywords: regression)

Attachments

(1 file)

[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.
Flags: needinfo?(mrbkap)
Tracking 55+ for this regression.
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.
Assignee: nobody → mrbkap
Flags: needinfo?(mrbkap)
Blocks: 1301056
Component: General → File Handling
OS: Unspecified → All
Product: Core → Firefox
Hardware: Unspecified → All
Version: Trunk → 55 Branch
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?
Attachment #8850782 - Flags: review?(mconley) → 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

> 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++!
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.
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".
Attachment #8850782 - Flags: review?(mconley) → 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
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
https://hg.mozilla.org/mozilla-central/rev/787274149cb6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
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]
Hi :mrbkap, 
Per comment #21 in bug 1301056, can you create an uplift request for Beta54?
Flags: needinfo?(mrbkap)
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]:
Flags: needinfo?(mrbkap)
Attachment #8850782 - Flags: approval-mozilla-beta?
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.
Attachment #8850782 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
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]
Depends on: 1368343
You need to log in before you can comment on or make changes to this bug.