New issue
Advanced search Search tips

Issue 811743 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

cloneNode of html5 muted video turns on the volume

Reported by sentimen...@gmail.com, Feb 13 2018

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.140 Safari/537.36

Steps to reproduce the problem:
1. Add a video tag with autoplay and muted attrs
2. Clone that video (cloneNode(true))
3. Audio starts playing! But we haven't even pasted the cloned elem to the page. It is rather weird

Example: http://tabasca.github.io/index.html

What is the expected behavior?
Clone evt doesn't need to set audio on.

What went wrong?
After cloning, clone's audio starts playing

Did this work before? Yes 63

Does this work in other browsers? No
 Can reproduce this bug in FF (found that it was broken from 41 version of FF!)
Other browsers (IE11, Edge, Safari 11) work as expected

Chrome version: 64.0.3282.140  Channel: stable
OS Version: 10.0
Flash Version: 

The only workaround is to manually set muted attr via javascript to the inserted clone-elem.
 
Link to that bug in FF (still not assigned to anyone q_q): https://bugzilla.mozilla.org/show_bug.cgi?id=1424871
Labels: Needs-Bisect Needs-Triage-M64
Cc: susan.boorgula@chromium.org
Components: Blink>Media>Video
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision ReleaseBlock-Stable Triaged-ET RegressedIn-64 M-64 Target-65 FoundIn-66 Target-66 FoundIn-64 FoundIn-65 Target-64 OS-Linux OS-Mac Pri-1
Owner: sriram...@samsung.com
Status: Assigned (was: Unconfirmed)
sentimentaleq@ Thanks for the issue.

Tested this issue on Mac OS 10.12.6, Windows 10 and Ubuntu 14.04 on the Stable 64.0.3282.167 and Canary 66.0.3347.0 and able to reproduce the issue by following the steps mentioned above.

Bisect Information:
===================
Good Build: 64.0.3254.0 (Revision - 512694)
Bad Build : 64.0.3255.0 (Revision - 513029)

On executing the per-revision bisect script, below is the Changelog URL:

https://chromium.googlesource.com/chromium/src/+log/6803a25bec5591feb3863bfe5e7aa7bf4b10a507..343baf357156bea80f4251c3df1a2ef931086e57

From the above Changelog, suspecting the below change:
Reviewed-on: https://chromium-review.googlesource.com/739262

srirama.m@ Please check and confirm if this issue is related to your change, else help us in assigning to the right owner.

Adding ReleaseBlock-Stable as this is a recent regression. Please feel to remove the same if it is not applicable.

Thanks.
Cc: mlamouri@chromium.org
The bisect information is correct.
Verified in firefox and the behaviour is same.
Labels: -M-64 -Target-64 M-65
Let's target this for M65.

Comment 6 by gov...@chromium.org, Feb 21 2018

M65 Stable promotion is coming VERY soon. Your bug is labelled as Stable ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Merge has to happen latest by 4:00 PM PT Monday (02/26/18) in order to make it to last M65 beta release next week. Thank you.
@Mounir, can we duplicate this to 813544 ?
This doesn't seem like an obvious duplicate because it's using clone() and not setting the attribute. Did I miss something?
Right, it is not exactly duplicate, though the change for the regression is same.
Now in this case the element is not added to tree, so the function ParserDidSetAttributes() never gets called and the muted attribute never gets set.
Interestingly it is same behaviour in mozilla.
Setting muted_ flag in loadResource() though solves this problem but there is one  failure in wpt test.
WDYT we should do now?
Gentle ping !!
It is marked as stable blocker for M65. Please merge fix ASAP as M65 stable promotion is scheduled today night.

Thanks..!
Labels: Hotlist-Interop
I can confirm that this is working as intended on Safari and Edge and broken on Chrome and Firefox.
+foolip@ FYI as muted is weird and he might want to know about the weird cases we are hitting for the spec.

srirama, I would suggest do override one of the Clone* methods from Element. Maybe `CloneAttributesFrom()`? This would be a quick fix but maybe not the best.

FWIW, the real issue here is that we use `ParserDidSetAttributes` to init the attributes but the clone code doesn't call this. Firefox is using something similar but I guess their cloning code re-uses the parser code. Safari is simply reading the muted state before playback like we used to.
Cc: foolip@chromium.org
+foolip for realz
Thanks @Mounir, i will try overriding the "CloseAttributesFrom()" method.
Does this fix needs to land tonight ?
Hmm, if the original element has a muted *content attribute* (not just the state bit set) then cloning it should work. In some part of the cloning, the attribute should be set on the new element, and that should set the internal state bit.

Details of fix unclear, but in agreement that this should work per spec.
Based on comment #10, the earlier the better. I'm happy to write the fix if it's too late in your timezone.
yes, can you please do it, thanks in advance.
Cc: sriram...@samsung.com
Owner: mlamouri@chromium.org
Status: Started (was: Assigned)
I've sent a CL for review to foolip@
Project Member

Comment 20 by bugdroid1@chromium.org, Feb 26 2018

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

commit 0d23cbfab31479d700140aa5d26d70d857f7be48
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Mon Feb 26 18:39:17 2018

HTMLMediaElement: set `muted_` when cloning element with muted attribute.

This CL works around the fact that cloning does not call the parser
callbacks and the HTML specifications require the attribute to only
have an effect when the element was created. There is unfortunately
no notification that the element creation from `cloneNode()` happens
so we instead rely on a callback meant for something slightly different.

Bug:  811743 
Change-Id: Ib7ac55088b9eaad23f9bce0cfc8366fec6fc5b0d
Reviewed-on: https://chromium-review.googlesource.com/937504
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#539207}
[modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/user-interface/muted.html
[modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp
[modify] https://crrev.com/0d23cbfab31479d700140aa5d26d70d857f7be48/third_party/WebKit/Source/core/html/media/HTMLMediaElement.h

Labels: TE-Verified-M66 TE-Verified-66.0.3356.0
Tested this issue on Windows 10 on the reported version 64.0.3282.140 and latest Canary 66.0.3356.0 by following the steps mentioned in the original comment.
Able to reproduce this issue on the reported version 64.0.3282.140 and the issue is fixed on the latest Canary 66.0.3356.0.

On loading the html page, the audio is not turned on after cloning.
Attached is the screen cast for reference.

Hence adding TE verified labels as the fix is working as intended.

Thanks..
811743-CL.mp4
1.6 MB View Download
Labels: Merge-Request-65
Requesting merge as it is a RBS.
Project Member

Comment 23 by sheriffbot@chromium.org, Feb 27 2018

Labels: -Merge-Request-65 Merge-Review-65 Hotlist-Merge-Review
This bug requires manual review: We are only 6 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
// correction in comment #21.

Tested the issue on Windows 10, Mac OS 10.12.6 and Ubuntu 14.04 and verifying the fix on all the 3 OS.

Thanks..
mlamouri@, How safe is the change to merge to M65? M65 is very close to stable promotion so we can only take critical and safe merge in at this point.
I think the change is safe but I do not know if it is worth a last minute merge. Issues with compatibility bugs like this is that we might hit Stable and discover that we broke a large website. If we can have one last Beta roll, I would prefer over directly hitting Stable.
Labels: -Merge-Review-65 Merge-Approved-65
Approving merge to M65 branch 3325 based on comments #21 and #26. Please merge ASAP so we can pick it up for last beta release tomorrow. Thank you.
Project Member

Comment 28 by bugdroid1@chromium.org, Feb 27 2018

Labels: -merge-approved-65 merge-merged-3325
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc6571e0d02d8098824bacbff599bcb02ca04e80

commit cc6571e0d02d8098824bacbff599bcb02ca04e80
Author: Mounir Lamouri <mlamouri@chromium.org>
Date: Tue Feb 27 16:13:15 2018

HTMLMediaElement: set `muted_` when cloning element with muted attribute.

This CL works around the fact that cloning does not call the parser
callbacks and the HTML specifications require the attribute to only
have an effect when the element was created. There is unfortunately
no notification that the element creation from `cloneNode()` happens
so we instead rely on a callback meant for something slightly different.

Bug:  811743 
Change-Id: Ib7ac55088b9eaad23f9bce0cfc8366fec6fc5b0d
Reviewed-on: https://chromium-review.googlesource.com/937504
Commit-Queue: Mounir Lamouri <mlamouri@chromium.org>
Reviewed-by: Philip Jägenstedt <foolip@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#539207}(cherry picked from commit 0d23cbfab31479d700140aa5d26d70d857f7be48)
Reviewed-on: https://chromium-review.googlesource.com/939561
Reviewed-by: Mounir Lamouri <mlamouri@chromium.org>
Cr-Commit-Position: refs/branch-heads/3325@{#605}
Cr-Branched-From: bc084a8b5afa3744a74927344e304c02ae54189f-refs/heads/master@{#530369}
[modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/LayoutTests/external/wpt/html/semantics/embedded-content/media-elements/user-interface/muted.html
[modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/Source/core/html/media/HTMLMediaElement.cpp
[modify] https://crrev.com/cc6571e0d02d8098824bacbff599bcb02ca04e80/third_party/WebKit/Source/core/html/media/HTMLMediaElement.h

Status: Fixed (was: Started)

Sign in to add a comment