MediaStreamTrack.applyConstraints will crash the tab if executed in quick succession
Reported by
johber...@gmail.com,
Aug 28 2017
|
||||||||||||||||||||||||||
Issue description
UserAgent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.59 Safari/537.36
Steps to reproduce the problem:
1. Use navigator.mediaDevices.getUserMedia to acquire a MediaStream and MediaStreamTrack.
2. Add the MediaStream to an HTML video element and delay for a short while, around 2.5 seconds.
3. execute MediaStreamTrack.applyConstraints({ advanced: [{ brightness: value }] }) *1000 times in quick succession
* in reality it has crashed in as little as 50 times, but a 1000 times should crash it most of the time.
What is the expected behavior?
The expected behavior is for the constraint to update without crashing the tab (Or Chrome App).
What went wrong?
If run locally in a tab, an 'Aw, Snap!' message will appear and DevTools will disconnect. If run in a Chrome App, the app will crash.
I've attached a screenshot of this.
Did this work before? Yes 60
Does this work in other browsers? N/A
Chrome version: 61.0.3163.59 Channel: beta
OS Version: 17.04
Flash Version:
I've tried this on Ubuntu Linux 17.04 and Chrome OS and it is the same on both operating systems. It works fine on Chrome 60 and previous versions but crashes on Chrome 61.
,
Aug 28 2017
I bisected this and tracked the problem to the following range: https://chromium.googlesource.com/chromium/src/+log/470096afdb28947722a3ecb946a4677f6ee9fa59..210bf791475500ca214d8111cda81da94bfd79ab Suspect change is r480447. Assigning to mcasas@ for further investigation.
,
Aug 28 2017
,
Sep 18 2017
johberlvi@ would you navigate to chrome://crashes after crashing and let us know the crash id? Thanks
,
Sep 20 2017
I posted it and it got deleted, I'll try again: Crash ID: crash/1c8df9715db93296
,
Sep 25 2017
guidou@ could you PTAL? Seems to be a generic problem with the autogenerated MediaTrack Constraint Set.
,
Sep 29 2017
The NextAction date has arrived: 2017-09-29
,
Sep 29 2017
The problem seems to be a null-pointer dereference when invoking |resolve_function| in OnMojoGetPhotoState(), at https://cs.chromium.org/chromium/src/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp?rcl=b6b9e4f8137ec1c2d1fd91df539de2746f642b81&l=691 I guess some bound argument becomes null. I get this trace: #0 0x7f15f3cbee57 base::debug::StackTrace::StackTrace() #1 0x7f15f3cbe92f base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7f15f3e44330 <unknown> #3 0x7f15ea883f0a WTF::String::String() #4 0x7f15eaa84bac blink::StringOrStringSequence::StringOrStringSequence() #5 0x7f15eaa90a25 blink::ConstrainDOMStringParameters::ConstrainDOMStringParameters() #6 0x7f15eaa8566c blink::StringOrStringSequenceOrConstrainDOMStringParameters::StringOrStringSequenceOrConstrainDOMStringParameters() #7 0x7f15eaa9218c blink::MediaTrackConstraintSet::MediaTrackConstraintSet() #8 0x7f15eaa92d4b blink::MediaTrackConstraints::MediaTrackConstraints() #9 0x7f15eac55427 _ZN4base8internal7InvokerINS0_9BindStateIMN5blink12ImageCaptureEFvNS3_21MediaTrackConstraintsEPNS3_21ScriptPromiseResolverEEJNS3_10PersistentIS4_EES5_EEEFvS7_EE3RunEPNS0_13BindStateBaseEOS7_ #10 0x7f15eac4cec8 blink::ImageCapture::OnMojoGetPhotoState() Assigning to reillyg@, who is an owner of ImageCapture for further investigation.
,
Oct 4 2017
The issue is that in ImageCapture::SetMediaTrackConstraints a MediaTrackConstraints is bound as an argument to a WTF::Function. Bound arguments don't get properly traced during garbage collection so its internal HeapVector<MediaTrackConstraintSet> is freed leading to a use-after-free when the Promise is eventually resolved.
,
Oct 4 2017
A fix is available in this patch: https://chromium-review.googlesource.com/c/chromium/src/+/701358 I have filed issue 771781 to track adding additional checks to prevent this class of issue in the future.
,
Oct 6 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7 commit 84ca1ee18bbc32f3cb035d071e8271e064dfd4d7 Author: Reilly Grant <reillyg@chromium.org> Date: Fri Oct 06 21:11:22 2017 Convert MediaTrackConstraints to a ScriptValue IDLDictionaries such as MediaTrackConstraints should not be stored on the heap which would happen when binding one as a parameter to a callback. This change converts the object to a ScriptValue ahead of time. This is fine because the value will be passed to a ScriptPromiseResolver that will converted it to a V8 value if it isn't already. Bug: 759457 Change-Id: I3009a0f7711cc264aeaae07a36c18a6db8c915c8 Reviewed-on: https://chromium-review.googlesource.com/701358 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/heads/master@{#507177} [modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html [modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp [modify] https://crrev.com/84ca1ee18bbc32f3cb035d071e8271e064dfd4d7/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
,
Oct 6 2017
,
Oct 6 2017
,
Oct 7 2017
,
Oct 8 2017
AFAICT, this is at least Sev_Medium, and thus a candidate for merging to M-62.
,
Oct 9 2017
Not seeing any crashes that appear to be related to this on Canary.
,
Oct 9 2017
,
Oct 9 2017
This bug requires manual review: We are only 7 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Oct 10 2017
+awhalley for security review.
,
Oct 10 2017
abdulsyed@ - good for 62
,
Oct 10 2017
Approving merge to M62. Branch:3202. Please merge before 4PM PDT to ensure it makes it to M62 Beta release candidate for tomorrow.
,
Oct 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/19765bd888949485b1e7ee2892b6502b1a653025 commit 19765bd888949485b1e7ee2892b6502b1a653025 Author: Reilly Grant <reillyg@chromium.org> Date: Tue Oct 10 20:27:35 2017 Convert MediaTrackConstraints to a ScriptValue IDLDictionaries such as MediaTrackConstraints should not be stored on the heap which would happen when binding one as a parameter to a callback. This change converts the object to a ScriptValue ahead of time. This is fine because the value will be passed to a ScriptPromiseResolver that will converted it to a V8 value if it isn't already. Bug: 759457 Change-Id: I3009a0f7711cc264aeaae07a36c18a6db8c915c8 Reviewed-on: https://chromium-review.googlesource.com/701358 Reviewed-by: Kentaro Hara <haraken@chromium.org> Commit-Queue: Reilly Grant <reillyg@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#507177}(cherry picked from commit 84ca1ee18bbc32f3cb035d071e8271e064dfd4d7) Reviewed-on: https://chromium-review.googlesource.com/710173 Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#642} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/LayoutTests/imagecapture/MediaStreamTrack-applyConstraints.html [modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/Source/modules/imagecapture/ImageCapture.cpp [modify] https://crrev.com/19765bd888949485b1e7ee2892b6502b1a653025/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
,
Oct 10 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e1f5e579ed01070bc96cdc87da69147393a4a10a commit e1f5e579ed01070bc96cdc87da69147393a4a10a Author: Reilly Grant <reillyg@chromium.org> Date: Tue Oct 10 22:00:57 2017 [M-62] Restore forward declaration of MediaTrackConstraints The cherry-pick in 19765bd888949485b1e7ee2892b6502b1a653025 removed the forward declaration of MediaTrackConstraints from ImageCapture.h. This was fine on head but this symbol was still used by another function prototype on branch 3202. Bug: 759457 Change-Id: Iba689e0e8222cb742d7eb2a8762e4b555bc3a256 Reviewed-on: https://chromium-review.googlesource.com/710531 Reviewed-by: Reilly Grant <reillyg@chromium.org> Cr-Commit-Position: refs/branch-heads/3202@{#649} Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098} [modify] https://crrev.com/e1f5e579ed01070bc96cdc87da69147393a4a10a/third_party/WebKit/Source/modules/imagecapture/ImageCapture.h
,
Oct 16 2017
,
Oct 18 2017
,
Nov 14 2017
,
Nov 16 2017
*** Boilerplate reminders! *** Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing. *********************************
,
Nov 16 2017
Nice one! The Chrome VRP Panel decided to award $1,000 for this report! A member of our finance team will be in touch to arrange for payment.
,
Nov 16 2017
,
Nov 21 2017
Thank you and your team, very much appreciated! :)
,
Jan 13 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Apr 25 2018
,
Jul 28
|
||||||||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||||||||
Comment 1 by guidou@chromium.org
, Aug 28 2017