1073872 - [e10s] downloading attachment over https results in corrupted file

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

Not sure whether this is a wider problem or something very specific, but I just tried to download an attachment from Gmail in a current Nightly (built from m-c revision 818f353b7aa6) with e10s enabled, and I got a corrupted file - the file is too small and its contents are, as far as I can tell, garbage.

The download manager reports the "wrong" size (matching the resulting file on disk), and suggests the download was successful, which suggests to me the problem is "upstream" from that code.

Opening a non-e10s window and downloading the same file works fine.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: Cyclemeter-Hike-20140927-0843.tcx

Hmm, I can reproduce in a clean profile (after enabling e10s), but it only happens with this specific file (a TCX file describing http://www.strava.com/activities/200255108).

Comment 2

[:Gavin Sharp [email: [e-mail]]]

(The other files I tried are a 4 byte file and a 512KB text file consisting of just "1" characters.)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

STR:
1) email attachment 8496479 [details] to a gmail account as an attachment
2 [review]) start e10s build with clean profile, log in to the gmail account
3) open message, download the attachment

Expected: Download manager shows download size as 413KB, file is XML
Actual: Download manager shows download size as 40.8KB, file is corrupted on disk

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Hrm, something weird is going on. I just tried the download in a Firefox 32.0.3 release build, and it completed successfully (file was 413KB, not corrupt), but the download size reported by the download manager was also 40.8KB (which matches the size of the "corrupt" file in the e10s case).

Comment 5

[Kevin Brosnan]

Works for me using - Mozilla/5.0 (Windows NT 6.3; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0 Today's build.

I tried a couple older pdfs that were in my gmail inbox and those downloaded fine as well.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

FWIW that bug manifests itself pretty differently from the comment there, so it might be worth testing separately.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

I'm still seeing this.

Comment 9

[Jim Mathies [:jimm]]

(In reply to :Gavin Sharp [email: [e-mail]] from comment #8)
> I'm still seeing this.

What kind of attachment? Any specific str you can add?

Comment 10

[Jim Mathies [:jimm]]

Not having much luck reproducing with various attachments, including the tcx file in comment 3 on mac and windows.

Comment 11

[Dave Townsend [:mossop]]

I can't reproduce this either.

Comment 12

[:Gavin Sharp [email: [e-mail]]]

Beyond comment 3, I've got nothing. This might be Google-account specific and related to cookie issues, as I theorized on IRC, so this could be tricky to debug...

Comment 14

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

better STR in https://bugzilla.mozilla.org/show_bug.cgi?id=1082134#c0

Comment 15

[cmtalbert]

Just to copy status from bug 1082134, I can still repro but only on the synergy download site behind the paywall (this is with today's nighlty build).

It's pretty cheap to get behind said paywall, so I recommend getting a copy and expensing it. Downloading the 1.5.x msi files seems to fail every time. Interestingly the synergy project nightly build page works every time (though it didn't a few weeks ago, so this bug's behavior is changing).

Comment 16

[Dragana Damjanovic [:dragana]]

There is a patch that landed a day after your comments that should fix you problem.

But looking at it, I was trying https://synergy-project.org/nightly/ I found another bug related to this one. Thanks.

Patch is coming.

Comment 17

[Dragana Damjanovic [:dragana]]

Attachment: fix v1

Hi Steve,

I am giving the review to you because you were working on it recently and I need a to ask you about a comment in you patch.

it is about proper encoding when we divert channel from parent back to child.
The problem with you patch is:
it diverts mParentListener in HttpChannelParent to a new listener. 
then DoApplyContentConversions is call and we have a new mConverterListener. So if RecvDivertOnDataAvailable are received the mConverterListener's OnDataAvailable is called and data are properly encoded.
After the channel is resume and HttpChannelParentListener's OnDataAvailable is called from the nsHttpChannel, HttpChannelParentListener has this new listener but it does not have the converters, so data are partially converted and partially not.

I was just wandering about the comment in http://hg.mozilla.org/mozilla-central/annotate/72ae882fce1a/netwerk/protocol/http/HttpChannelParent.cpp#l1009 .. 1016.
I am not sure how the first option is reflected in your code, I do not see it at all. Can you explain it to me, thanks.

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Comment on attachment 8517589 [details] [diff] [review]
fix v1

Review of attachment 8517589 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dragana Damjanovic from comment #17)
> ...
> After the channel is resume and HttpChannelParentListener's OnDataAvailable
> is called from the nsHttpChannel, HttpChannelParentListener has this new
> listener but it does not have the converters, so data are partially
> converted and partially not.

Good catch - yes, my previous patch did not add the decoder listener chain to HttpChannelParentListener for ResumeAfterDiversion.

> I was just wandering about the comment in
> http://hg.mozilla.org/mozilla-central/annotate/72ae882fce1a/netwerk/protocol/
> http/HttpChannelParent.cpp#l1009 .. 1016.
> I am not sure how the first option is reflected in your code, I do not see
> it at all. Can you explain it to me, thanks.

It's not ;) Can you update the comments to make sure they reflect your changes, please? r=me with comment updates.

Comment 19

[Dragana Damjanovic [:dragana]]

Attachment: fix v1

Only comment updated.

Comment 21

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=259f4e9a4530

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a33eb2f32a32

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a33eb2f32a32

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

There don't seem to have been any more reports of this issue after the fix.