Closed Bug 1073872 Opened 5 years ago Closed 5 years ago

[e10s] downloading attachment over https results in corrupted file

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
e10s m4+ ---

People

(Reporter: Gavin, Assigned: dragana)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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).
(The other files I tried are a 4 byte file and a 512KB text file consisting of just "1" characters.)
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
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).
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.
Status: NEW → RESOLVED
Closed: 5 years ago
tracking-e10s: ? → ---
Resolution: --- → DUPLICATE
Duplicate of bug: 1065042
FWIW that bug manifests itself pretty differently from the comment there, so it might be worth testing separately.
I'm still seeing this.
Status: RESOLVED → REOPENED
tracking-e10s: --- → ?
Resolution: DUPLICATE → ---
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #8)
> I'm still seeing this.

What kind of attachment? Any specific str you can add?
Flags: needinfo?(gavin.sharp)
Not having much luck reproducing with various attachments, including the tcx file in comment 3 on mac and windows.
I can't reproduce this either.
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...
Flags: needinfo?(gavin.sharp)
Assignee: nobody → jmathies
Duplicate of this bug: 1082134
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).
Summary: [e10s] downloading attachment from Gmail results in corrupted file → [e10s] downloading attachment over https results in corrupted file
Assignee: jmathies → dd.mozilla
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.
Attached patch fix v1 (obsolete) — Splinter Review
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.
Attachment #8517589 - Flags: review?(sworkman)
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.
Attachment #8517589 - Flags: review?(sworkman) → review+
Attached patch fix v1Splinter Review
Only comment updated.
Attachment #8517589 - Attachment is obsolete: true
Attachment #8518952 - Flags: review+
Duplicate of this bug: 1095804
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a33eb2f32a32
Status: REOPENED → RESOLVED
Closed: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1097878
Depends on: 1106396
There don't seem to have been any more reports of this issue after the fix.
Flags: qe-verify-
Keywords: qawanted
You need to log in before you can comment on or make changes to this bug.