Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Upload progress incorrect in Google Chrome #1016

Merged
merged 2 commits into from
Nov 10, 2023
Merged

Upload progress incorrect in Google Chrome #1016

merged 2 commits into from
Nov 10, 2023

Conversation

mkszepp
Copy link
Contributor

@mkszepp mkszepp commented Nov 8, 2023 •
edited by gilest
Loading

Sometimes the browser doesn't has the correct file size onloadstart. like described here: #1002 (comment)

During debugging i have found out, that this bug is present in Chrome. In Chrome there is often a total wrong total size (see comment) and in Firefox there is sometime a little difference between the total size onloadstart and onprogress.

We should set file.size like in earlier versions also in onprogress event and check in progress event, if the upload is already complete.

#1013

Progress log on Chrome with this fix (3 files in queue):

0
0
1
1
3
3
5
5
6
6
8
8
10
10
11
11
13
13
15
15
16
16
18
18
20
20
21
21
23
23
24
24
24
24
24
24
24
24 // first file uploaded
26
26
27
27
29
29
31
31
33
33
34
34
36
36
38
38
39
39
41
41
43
43
44
44
46
46
48
48
49
49
51
51
53
53
55
55
56
56
58
58
60
60
61
61
63
63
65
65
66
66
68
68
70
70
71
71
73
73
75
75
76
76
78
78
80
80
82
82
83
83
85
85
87
87
88
88
88
88
88
88
88 // 2nd file uploaded
88
89
89
91
91
93
93
95
95
96
96
98
98
100
100
100
100 // all files uploaded

Progress log in Firefox after fix (3 files in queue):

3
6
9
12
15
19
22
24
24 // first file uploaded
27
31
34
37
40
43
47
50
53
56
59
62
66
69
72
75
78
82
85
86
86 // 2nd file uploaded
89
92
95
98
100 // all files uploaded

@mkszepp mkszepp mentioned this pull request Nov 8, 2023
Merged
@RobbieTheWagner
Copy link
Member

@mkszepp would it be possible to add some tests for checking progress values? We could then run those tests in both Chrome and Firefox to catch regressions here.

@jelhan
Copy link
Collaborator

jelhan commented Nov 9, 2023

We could then run those tests in both Chrome and Firefox to catch regressions here.

I'm not sure if we can test the difference at this level. If I understand correctly, both Chrome and Firefox may have a wrong file size in onloadstart event. I fear we are mocking that layer in the tests:

ember-file-upload/ember-file-upload/src/mirage/upload-handler.ts

Lines 55 to 61 in c3af01e

request.upload.onloadstart(
new ProgressEvent('loadstart', {
lengthComputable: true,
total,
loaded: Math.min(loaded, total),
}),
);

@jelhan
Copy link
Collaborator

jelhan commented Nov 9, 2023

I had a look in specification: https://xhr.spec.whatwg.org/#the-send()-method

If I understand correctly, the total of ProgressEvent fired at loadstart should be either 0 or the actual length of the body. See step 11. But if I understand correctly, we see different behavior of the browsers in reality, correct?

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 9, 2023 •
edited
Loading

@jelhan exact, we have specially on Chrome the behavior, that loadstart brings a the wrong total. I think this problem occurs with large files, small files are not the problems (because there was 1 of 3 files with this bug)...
In Firefox i have got also yesterday with one file a little difference (but not always).

I have tested this fix today with a 30 MB image in "Fast 3G" mode and it works.

Edit:
I will try to make some tests in the next days

@RobbieTheWagner
Copy link
Member

@mkszepp great, thanks! Please keep me posted on writing tests. I do think we should probably merge this either way, but would love to have regression coverage.

@gilest
Copy link
Collaborator

gilest commented Nov 9, 2023 •
edited
Loading

Thank you @mkszepp – this looks like an improvement. I'll try to stand up tests

I'm not sure if we can test the difference at this level. If I understand correctly, both Chrome and Firefox may have a wrong file size in onloadstart event. I fear we are mocking that layer in the tests

Yes, that's why I am requesting (in #1013) that we record some upload event data from different browsers and endpoints so that we can re-arrange some internals and replay those events in unit/integration tests

Testing this with the existing mirage handler is useless because the browser and endpoint behaviour is mocked

@gilest gilest added the bug label Nov 9, 2023
@gilest gilest changed the title Fix upload progress Upload progress incorrect in Google Chrome Nov 9, 2023
@gilest
Copy link
Collaborator

gilest commented Nov 10, 2023

Pretty sure I've validated with unit tests that this fix is correct – at least for this specific issue (progress straight to 100% in Google Chrome). I'll open a proof PR shortly

@gilest gilest merged commit ccf2363 into adopted-ember-addons:master Nov 10, 2023
14 checks passed
@gilest gilest mentioned this pull request Nov 10, 2023
Merged
@mkszepp mkszepp deleted the fix-upload-progress branch November 10, 2023 08:11
gilest added a commit that referenced this pull request Nov 10, 2023
@gilest
Replay progress events in unit tests (#1017)
afd70da 
Skips the browser, http, network etc. and lets us test the smallest unit of our internals – an UploadFile having its state updated by a series of ProgressEvent events.

For each scenario I'm pretty sure we only need to test a single file with a stream of synchronous events.

Covers some recorded events from different browsers and endpoints. We can add more scenarios here if we come across them.

Specifically, the Chrome scenario proves the fix in #1016 – this was previously failing early with 100% progress, which is the same issue reported by users in #1013

I've painstakingly copied values from log screenshots to setup this existing suite. Hopefully contributors can PR full tests in future to make things a bit easier 😅

Fixes #1013
@gilest
Copy link
Collaborator

gilest commented Nov 10, 2023

Thanks again.

Fix released in 8.3.1

@mkszepp
Copy link
Contributor Author

mkszepp commented Nov 13, 2023

Thanks! i have tested in my app and works!

@RobbieTheWagner maybe you can give us also a short feedback, if the bug in you app is also resolved with this fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mkszepp @RobbieTheWagner @jelhan @gilest