The connection was reset for partial content

RESOLVED FIXED in Firefox -esr60

Status

()

P2
normal
Rank:
10
RESOLVED FIXED
a year ago
5 months ago

People

(Reporter: euthanasia_waltz, Assigned: jya)

Tracking

(Blocks: 2 bugs, {regression})

59 Branch
mozilla62
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr6061+ fixed, firefox60- wontfix, firefox61+ fixed, firefox62+ fixed)

Details

Attachments

(3 attachments)

(Reporter)

Description

a year ago
STR:
1. Go to http://media.ca7.uscourts.gov/oralArguments/oar.jsp?caseyear=&casenumber=&period=Past+week
2. Click any oral argument

AR:
'The connection was reset' page appears after a while.(within a few minutes)

ER:
Playback to the end.

mozregression:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=0ff4a36cce94d0b9cb64c3ee7204e72749707e5c&tochange=fdb2a41005f2c8ae4d3528c893821f9eb06b3f0c

Former good case, firefox was fetching remained data of partial content. However, for current bad case, firefox is not fetching it.
(Reporter)

Updated

a year ago
Blocks: 1415090
Confirming and setting to NEW

Playback will work up to 30-45 seconds before the error occurs.  Reloading the page seems to allow the mp3 to play, I let it run for over 2 mins before the error occurred again.

No errors in the Browser Console
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: regression
Jya: who would be a good person to take a look at this?
No longer blocks: 1415090
Rank: 10
Depends on: 1415090
Flags: needinfo?(jyavenard)
Priority: -- → P2

Updated

11 months ago
Blocks: 1454247
(Assignee)

Comment 3

10 months ago
(In reply to Nils Ohlmeier [:drno] from comment #2)
> Jya: who would be a good person to take a look at this?

I can't think of any as of today...
Flags: needinfo?(jyavenard)
(Assignee)

Comment 4

10 months ago
I'm going to take this one. This looks very serious to me as it would affect any long mp3 played (and likely any long files)

At a glance, the change in bug 1415090, P2 changed from a synchronous seek to an async one. The next call following the OnTransferEnded see a closed stream and abort, while the synchronous seek would have reopened the stream immediately.
(Assignee)

Comment 5

10 months ago
Regression reports are typically puts as blocker of the bug that introduced the regression.
Blocks: 1415090
No longer blocks: 1454247
No longer depends on: 1415090
See Also: → bug 1454247
(Assignee)

Updated

10 months ago
Assignee: nobody → jyavenard
(Assignee)

Comment 6

10 months ago
With bug 1415090 reverted, while I don't get a connection was reset error, playback typically stalls completely.... So it's likely the problem isn't just with 1415090 but with a change that occurred earlier.
(Assignee)

Comment 7

10 months ago
STR (not that I can't reproduce it on a linux laptop, but always on my mac pro or in VM)

Go to preferences, select "Privacy & Security" 
Click on "Clear Data", make sure only "Cached Web content" is selected, click on "Clear"

now open: http://media.ca7.uscourts.gov/sound/external/rt.17-2132.17-2132_05_17_2018.mp3

Wait between 1m30s and 4m : the message will appear.

The regression first started with this commit: https://hg.mozilla.org/mozilla-central/rev/94287653dc31

Where ChannelMediaResource::OnStopRequest was changed to perform the call to Seek() synchronously, and instead then called mCacheStream.NotifyDataEnded(aStatus); which in turned called ChannelMediaResource::CacheClientSeek(mChannelOffset, false);

CacheClientSeek queue a task on the main thread to ChannelMediaResource::Seek.

So the regression was introduced as the ChannelMediaResource::Seek is no longer called immediately. Seek closes the http channel to re-open it immediately.

With a rr recording, I placed a breakpoint on ChannelMediaResource::CacheClientSeek and one on the lambda calling seek. I then added a breakpoint to every ChannelMediaResource entries to determine if any calls were made between the time the seek was called and the time it runs: the answer is negative.

Gerald, you last worked with this code, and reviewed JW's changes. I would very much appreciate if you could have a go, this is driving me insane.
Flags: needinfo?(gsquelart)
(Assignee)

Updated

10 months ago
Blocks: 1464045
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

10 months ago
[Tracking Requested - why for this release]: this is a serious issue if ever attempting to play long content. should be uplifted.
status-firefox60: --- → affected
status-firefox61: --- → affected
tracking-firefox61: --- → ?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 16

10 months ago
mozreview-review
Comment on attachment 8980267 [details]
Bug 1450607 - P1. Fix constness.

https://reviewboard.mozilla.org/r/246426/#review252740
Attachment #8980267 - Flags: review?(gsquelart) → review+
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → affected
tracking-firefox60: --- → ?
tracking-firefox61: ? → +
tracking-firefox62: --- → +
tracking-firefox-esr60: --- → ?

Comment 17

10 months ago
mozreview-review
Comment on attachment 8980268 [details]
Bug 1450607 - P2. Synchronously seek to prepare for resuming following stop request.

https://reviewboard.mozilla.org/r/246428/#review252744

::: dom/media/MediaCache.cpp:2423
(Diff revision 4)
> +Pair<int64_t,int64_t>
> +MediaCacheStream::GetLengthAndOffset() const
> +{
> +  MOZ_ASSERT(NS_IsMainThread());
> +  AutoLock lock(mMediaCache->Monitor());
> +  return Pair<int64_t, int64_t>(mStreamLength, mChannelOffset);

In this case (calling a constructor by explicitly specifying the exact same type as the return type) you could just do `return { mSL, mCO };` for the same effect without redundant type.
Of course, it's a question of taste whether you actually want to show the type again, for extra clarity; so you may leave it as-is if you prefer.
(Sorry if you already knew that.)
Attachment #8980268 - Flags: review?(gsquelart) → review+

Comment 18

10 months ago
mozreview-review
Comment on attachment 8980333 [details]
Bug 1450607 - P3. Remove unused argument.

https://reviewboard.mozilla.org/r/246482/#review252748
Attachment #8980333 - Flags: review?(gsquelart) → review+
Flags: needinfo?(gsquelart)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 22

10 months ago
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0ef665046a06
P1. Fix constness. r=gerald
https://hg.mozilla.org/integration/autoland/rev/2fce9c07ec2b
P2. Synchronously seek to prepare for resuming following stop request. r=gerald
https://hg.mozilla.org/integration/autoland/rev/4688aa455ae3
P3. Remove unused argument. r=gerald

Comment 23

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/0ef665046a06
https://hg.mozilla.org/mozilla-central/rev/2fce9c07ec2b
https://hg.mozilla.org/mozilla-central/rev/4688aa455ae3
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
tracking-firefox-esr60: ? → 61+
Flags: qe-verify+
(Assignee)

Comment 24

10 months ago
Comment on attachment 8980267 [details]
Bug 1450607 - P1. Fix constness.

Approval Request Comment
[Feature/Bug causing the regression]: 1415090
[User impact if declined]: Can't play some media files (typically the ones longer than a few minute), the code introduced in bug 1415090 was racy
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: tested manually
[Needs manual test from QE? If yes, steps to reproduce]: The STR were provided in comment 7
[List of other uplifts needed for the feature/fix]: all patches attached to this bug
[Is the change risky?]: likely not.
[Why is the change risky/not risky?]: We revert to an earlier method. 
[String changes made/needed]: none
Attachment #8980267 - Flags: approval-mozilla-esr60?
Attachment #8980267 - Flags: approval-mozilla-beta?
Comment on attachment 8980267 [details]
Bug 1450607 - P1. Fix constness.

Fix for possible playback issues of long media files. Approved for 61.0b10 and ESR 60.1.
Attachment #8980267 - Flags: approval-mozilla-esr60?
Attachment #8980267 - Flags: approval-mozilla-esr60+
Attachment #8980267 - Flags: approval-mozilla-beta?
Attachment #8980267 - Flags: approval-mozilla-beta+
I've tested on Windows 7 x64 and macOS 10.13 using latest Nightly 62.0a1 (2018-05-29) and the CI builds from  https://treeherder.mozilla.org/#/jobs?repo=mozilla-beta and I still reproduce the issue: after 1-2 minutes, 'The connection was reset' page appears. 
I used the STR provided in comment 7. 

Any thoughts?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 29

10 months ago
what version of the tree was that build using?
Flags: needinfo?(jyavenard)
(Assignee)

Comment 30

10 months ago
I can't reproduce in 62.0a1 (2018-05-30) (64-bit)

2018-05-29 probably doesn't have the fix yet.
I've retested on Windows 7 x64 and Windows 10 x64 using Nightly 62.0a1 (2018-05-30) and Firefox 61 Beta 10 x64 and x32 builds(buildID: 20180530184300) and the issue is still reproducible. 

Please see here how I reproduced the issue: https://imgur.com/BU09E0C .
(Assignee)

Comment 32

10 months ago
Thanks for checking on that.

Could you run mozregression on that machine to find out when this broke, and if maybe there's another problem at play (we would have only fixed one)
Flags: needinfo?(camelia.badau)
(Assignee)

Comment 33

10 months ago
I kicked out an old Windows 7 VM (32 bits) can't reproduce it in there either...

Could with an earlier release :(
Unfortunately, mozregression can't generate me a pushlog ("Unable to find enogh data to bisect" error).
I found that the last good build is 2017-05-05 (buildID: 20170505030252) and first bad build is 2017-05-06 (buildID: 20170506030204). I hope this helps you.
Flags: needinfo?(camelia.badau)
(Assignee)

Comment 35

10 months ago
this is great thank you...

So this is a different regression than bug 1415090 which is what this bug fixed. need to find the culprit now
Flags: needinfo?(jyavenard)
(Assignee)

Updated

10 months ago
Blocks: 1466569
status-firefox60: affected → wontfix
tracking-firefox60: ? → -
Hi Jean-Yyves,

Considering your comment 7 and Camelia's comment 28, comment 31 and comment 34, I am unsure what fix should be verified in this issue: for the purpose of reevaluating the need for manual qe verification on this bug I am setting the qe-verify flag to ?.
Flags: qe-verify+ → qe-verify?
(Assignee)

Comment 37

10 months ago
So we have two bugs.

The first one was fixed here, the other will be tracked in bug 1466569 (it's a year old bug, and likely was existing before that just hidden)
Flags: needinfo?(jyavenard)
Let's leave qe-verify+ on, and revisit this bug for verification after bug 1466569 is fixed.
Flags: qe-verify? → qe-verify+
Here, I think we wanted to verify the issue in nightly 62 back in May, for more certainty for uplifting to beta 61.  At this point, verifying the issue won't affect our decisions, since the fix already landed in 61 and 62.  So, you may not want to make this a priority for verification.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.