New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 730647 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 1
Type: Bug



Sign in to add a comment

Web MIDI: data loss on receiving continuous sysex on Chrome 58+ on Windows

Project Member Reported by toyoshim@chromium.org, Jun 7 2017

Issue description

Chrome Version: Chrome 58+
OS: Windows

What steps will reproduce the problem?
(1) Install (or use) a loopback thing (such as http://www.tobias-erichsen.de/software/loopmidi.html)
(2) Open up https://jsfiddle.net/toyoshim/wjvjnqzx/ (and open the dev console)
(3) See console

What is the expected result?
No error log

What happens instead?
Data loss is detected

 
Now I have a fix, and try to submit and merge to release branches as I can as possible.

Comment 2 by j...@krutisch.de, Jun 7 2017

Thanks for the Quick help!

Can I just state for the record (if that's able to add any weight in terms of stable branch merging) that this currently very likely breaks production code on at least two rather popular customer facing applications: 

- https://components.novationmusic.com
- https://soundmondo.yamahasynth.com/

both are applications to manage content of hardware synthesizers and are somewhat without alternatives for the users.

(I reported the bug originally and I am one of the maintainers of Novation Components)

Thank you, Jan

Let me share a workaround for advanced users who really need a quick fix.

Flip an experimental flag at chrome://flags/#use-winrt-midi-api to Enabled.
This will switch to use New Windows 10 API to manage MIDI devices, but there is a known issue that if users use other applications that call legacy MIDI API together.
Once this conflict happens, Windows MIDI system may be confused, and will require
system reboot to use MIDI system again.

I will try to fix the problem as soon as possible, and try merging the fix to release branches. Fix itself is very simple, but I can not promise if my fix can meet
the bar for merging to m59 Stable.
I forgot to explain, but chrome://flags/#use-winrt-midi-api works only on Windows 10, and we do not have any workaround on other Windows versions such as 7 and 8.
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 7 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1814e6af281cd1f00ddf0705c01e2484708b0e4d

commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d
Author: toyoshim <toyoshim@chromium.org>
Date: Wed Jun 07 23:46:03 2017

Web MIDI: restore sysex receiving buffer synchronously

The new backend restores sysex receiving buffer asynchronously,
but it turned out that this way causes a receiving data loss on
receiving continuous sysex messages.

The original backend did it synchronously and didn't have such
problem. So let me fix the new backend to take the same approach.

BUG= 730647 

Review-Url: https://codereview.chromium.org/2926143003
Cr-Commit-Position: refs/heads/master@{#477807}

[modify] https://crrev.com/1814e6af281cd1f00ddf0705c01e2484708b0e4d/media/midi/midi_manager_win.cc

Labels: Merge-Request-59 Merge-Request-60
Project Member

Comment 7 by sheriffbot@chromium.org, Jun 8 2017

Labels: -Merge-Request-59 Merge-Review-59 Hotlist-Merge-Review
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
hi toyoshim@ - thanks for the quick bug fix. My recommendation is to wait for M60. It appears that there is a workaround and the impact doesn't seem very large. Since M59 is already being pushed to stable users, the bar is very high for new merges. If you think this is absolutely critical and can not wait until M60, please let us know. 

Comment 9 by j...@krutisch.de, Jun 8 2017

hi abdulsyed@ - toyoshim@ told me that this is a possible outcome, but I must say as the original reporter that I am a bit disappointed.

I'm not sure what counts as high impact (I assume you judge that by affected users, which is fair, I guess) but we have real users (clients, actually) that use our software every day as professional and hobbyist musicians. Our software is broken for them on Chrome and it is quite an ask for me to point them to a chrome flag that potentially breaks MIDI on their systems on a regular basis and forces them to reboot.

My main gripe is that we're only a few days after the last stable release, so we'll have to wait about 6 weeks (that's the current release cycle, right?) to get a fix out to users while at the same time be afraid that the bug spreads to our fallbacks (Opera et al.) and we'll be without any fallback at all for a couple of weeks.

Anyway, I totally understand that there are calls to be made and they are difficult and all, and I'm also really happy toyoshim@ was able to fix this so quickly, but I hope you can also understand my frustration about having to wait so long to get a fix that will actually work for my users.

(I'll shut up now and let y'all do your job)
abdulsyed@ thank you for comments.

By the way, why this is not approved automatically even for M-60?


Also, for M-59 request, let me explain a little more background to answer questions too, though Jan already provided a precious feedback and great explanations.

> It appears that there is a workaround
The workaround requires to flip an unsupported experimental flag at chrome://flags, and we already noticed that this flag should be used carefully, because Windows gets confused and stop working until rebooting the operating system. Also it's supported only on Windows 10. So it isn't a solution for typical end users.

> the impact doesn't seem very large
The number of affected people should be limited. That's right. But for users who has used the service and products Jan mentioned, this problem makes their instruments and services completely useless over a month. So the problem is serious for them.

I think this should not be an issue that requires a stable refresh only for this fix, but want to ride on a train together if there are other fixes and chance to ride.

The patch itself is very simple one, that removes 3 lines, and adds 1 line.

That's all my explanations, and will wait for responsible person's decision.
Labels: -Merge-Review-59 -Merge-Request-60 Merge-Approved-59 Merge-Approved-60
OK great, thank you so much for the feedback. Based on comment10, approving merge for M59 and M60. 
Labels: -Merge-Approved-59 Merge-Request-59
can you please confirm if this is well tested/verified in canary?
Project Member

Comment 13 by sheriffbot@chromium.org, Jun 8 2017

Labels: -Merge-Request-59 Merge-Review-59
This bug requires manual review: Request affecting a post-stable build
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), gkihumba@(ChromeOS), Abdul Syed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Hi abdulsyed, is the branch for the 60 3112, right?

Jan, I can verify Canary working with soundmondo.
Could you verify it with components?
yes, it's 3112 for m60. 
Project Member

Comment 16 by bugdroid1@chromium.org, Jun 9 2017

Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7829724b2cb160c6d96f17bb8738d4c58c5ecd6f

commit 7829724b2cb160c6d96f17bb8738d4c58c5ecd6f
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Fri Jun 09 07:06:35 2017

Web MIDI: restore sysex receiving buffer synchronously

The new backend restores sysex receiving buffer asynchronously,
but it turned out that this way causes a receiving data loss on
receiving continuous sysex messages.

The original backend did it synchronously and didn't have such
problem. So let me fix the new backend to take the same approach.

BUG= 730647 

(cherry picked from commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d)

Review-Url: https://codereview.chromium.org/2926143003
Cr-Original-Commit-Position: refs/heads/master@{#477807}
Change-Id: I493750ac10ab2ece46fa40aa05a3ae296ea1d6f7
Reviewed-on: https://chromium-review.googlesource.com/527546
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#274}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/7829724b2cb160c6d96f17bb8738d4c58c5ecd6f/media/midi/midi_manager_win.cc

Comment 17 by j...@krutisch.de, Jun 9 2017

Can verify Novation Components is working fine with the current win32 canary.

Thanks again for the super speedy fix everyone!
Labels: -Merge-Review-59 Merge-Approved-59
Thanks for confirming - approving merge for M59. Branch is 3071.
Please make sure to merge before Sunday 5pm (pacific time) to make RC for next stable release. 
I just confirmed soundmondo too, and will submit a merge change soon.
Project Member

Comment 21 by bugdroid1@chromium.org, Jun 10 2017

Labels: -merge-approved-59 merge-merged-3071
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f6e6469cc6ccb2b6a5d9c843754d76f25a17d133

commit f6e6469cc6ccb2b6a5d9c843754d76f25a17d133
Author: Takashi Toyoshima <toyoshim@chromium.org>
Date: Sat Jun 10 03:07:58 2017

Web MIDI: restore sysex receiving buffer synchronously

The new backend restores sysex receiving buffer asynchronously,
but it turned out that this way causes a receiving data loss on
receiving continuous sysex messages.

The original backend did it synchronously and didn't have such
problem. So let me fix the new backend to take the same approach.

BUG= 730647 

(cherry picked from commit 1814e6af281cd1f00ddf0705c01e2484708b0e4d)

Review-Url: https://codereview.chromium.org/2926143003
Cr-Original-Original-Commit-Position: refs/heads/master@{#477807}
Change-Id: I72470e9c91754f6858dcd513c7d4a527411097b4
Reviewed-on: https://chromium-review.googlesource.com/527783
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Cr-Commit-Position: refs/branch-heads/3071@{#770}
Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641}
[modify] https://crrev.com/f6e6469cc6ccb2b6a5d9c843754d76f25a17d133/media/midi/dynamically_initialized_midi_manager_win.cc

Status: Fixed (was: Started)
Submitted a merge fix, that needed a manual rebase due to a file rename.
Cc: krajshree@chromium.org
Labels: Needs-Feedback
Tested the issue on Win-10 using chrome beta version #60.0.3112.32.

Attached a screen cast for reference.

Following are the steps followed to reproduce the issue.
------------
1. Installed a loopmidi from URL: http://www.tobias-erichsen.de/software/loopmidi.html
(2) Opened https://jsfiddle.net/toyoshim/wjvjnqzx/ in chrome browser and opened the dev console.
(3)Observed error log in dev console.

toyoshim@ - Could you please verify the screen cast and please confirm the expected behavior.

Thanks...!!
730647.mp4
1.9 MB View Download
You added a virtual port with the name "loopMIDI Port1", but could you change the name to be "loopMIDI Port" that the test expects? (the last "1" should be removed) In your movie the test fails to find the test device.
Labels: TE-Verified-M60 TE-Verified-60.0.3112.32
toyoshim@ - Thanks for clarification...!!

Verified the issue on windows 10 using chrome beta version #60.0.3112.32 as per comment #0.
Observed No error log in dev console. Hence, the fix is working as expected.

Attaching screen shot for reference.

Hence, adding the verified labels.

Thanks...!!
730647.jpeg
176 KB View Download
Thanks, the last screen shot proves the test runs correctly!
Labels: TE-Verified-M59 TE-Verified-59.0.3071.104
Verified this issue on Windows-10 using chrome latest stable #59.0.3071.104 by following steps mentioned in the original comment and observed no error log in the console as expected. Hence adding TE-Verified label.

Thanks!
730647.jpg
167 KB View Download
thank you so much!

Sign in to add a comment