145986 2015-06-15 13:25:12 -0700 Media Session: Improve the safety of playback toggling 2015-06-15 16:06:48 -0700 1 1 1 Unclassified WebKit Media 528+ (Nightly build) All All RESOLVED FIXED InRadar P2 Normal --- 145411 1 mrajca webkit-unassigned achristensen commit-queue conrad_shultz darin eric.carlson jer.noble mrajca webkit-bug-importer oldest_to_newest 1102020 0 mrajca 2015-06-15 13:25:12 -0700 Per Darin's feedback in Bug 145978, we should be careful about iterating elements that were deleted underneath us in MediaSession::togglePlayback. 1102022 1 webkit-bug-importer 2015-06-15 13:26:43 -0700 <rdar://problem/21388739> 1102024 2 254891 mrajca 2015-06-15 13:38:40 -0700 Created attachment 254891 Patch 1102027 3 254891 achristensen 2015-06-15 13:42:51 -0700 Comment on attachment 254891 Patch There's no need to make a member variable for something that's only used in MediaSsession::togglePlayback. 1102029 4 mrajca 2015-06-15 13:50:43 -0700 (In reply to comment #3) > Comment on attachment 254891 [details] > Patch > > There's no need to make a member variable for something that's only used in > MediaSsession::togglePlayback. We will have to remove elements from that set when elements from the underlying m_activeParticipatingElements set are removed. 1102032 5 achristensen 2015-06-15 14:08:14 -0700 It would be much nicer to not use so many raw pointers everywhere. 1102059 6 254891 darin 2015-06-15 15:10:18 -0700 Comment on attachment 254891 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=254891&action=review > Source/WebCore/Modules/mediasession/MediaSession.cpp:97 > + ASSERT(!m_iteratedActiveParticipatingElements); What guarantees this won’t happen? I’d think that if we make something play that some events might fire and in response we might run some code and end up back here. Maybe we need to handle this case, perhaps by doing nothing? 1102060 7 mrajca 2015-06-15 15:14:01 -0700 (In reply to comment #6) > Comment on attachment 254891 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254891&action=review > > > Source/WebCore/Modules/mediasession/MediaSession.cpp:97 > > + ASSERT(!m_iteratedActiveParticipatingElements); > > What guarantees this won’t happen? I’d think that if we make something play > that some events might fire and in response we might run some code and end > up back here. > > Maybe we need to handle this case, perhaps by doing nothing? This assertion is in the MediaSession::togglePlayback method which is currently only called in response to play/pause media keys. This method will never call itself. 1102061 8 254891 darin 2015-06-15 15:18:15 -0700 Comment on attachment 254891 Patch Oops, just noticed this doesn’t add the needed call to remove. Why not? 1102065 9 mrajca 2015-06-15 15:22:10 -0700 We cur(In reply to comment #8) > Comment on attachment 254891 [details] > Patch > > Oops, just noticed this doesn’t add the needed call to remove. Why not? We currently don't remove elements from m_activeParticipatingElements (and thus m_iteratedActiveParticipatingElements). That's part of the algorithm for "interrupting a media session from a media element" which I'll implement in the next couple days. 1102069 10 achristensen 2015-06-15 15:27:22 -0700 (In reply to comment #9) > We currently don't remove elements from m_activeParticipatingElements (and > thus m_iteratedActiveParticipatingElements). That's part of > the algorithm for "interrupting a media session from a media element" which > I'll implement in the next couple days. This is why I was so confused. This will need to be done, but 185560 doesn't need it yet. 1102074 11 darin 2015-06-15 15:41:28 -0700 (In reply to comment #9) > We cur(In reply to comment #8) > > Comment on attachment 254891 [details] > > Patch > > > > Oops, just noticed this doesn’t add the needed call to remove. Why not? > > We currently don't remove elements from m_activeParticipatingElements (and > thus m_iteratedActiveParticipatingElements). That's part of > the algorithm for "interrupting a media session from a media element" which > I'll implement in the next couple days. Before that algorithm comes into play: What guarantees that m_activeParticipatingElements doesn’t point to deleted objects when media elements go away? 1102075 12 mrajca 2015-06-15 15:48:24 -0700 Good catch. Section 6.5 of the Media Session spec describes what should happen when an element is removed from the document (which includes removing it from the media session's set of active participating elements). I'll get to that soon as well. 1102080 13 254891 commit-queue 2015-06-15 16:06:41 -0700 Comment on attachment 254891 Patch Clearing flags on attachment: 254891 Committed r185570: <http://trac.webkit.org/changeset/185570> 1102081 14 commit-queue 2015-06-15 16:06:48 -0700 All reviewed patches have been landed. Closing bug. 254891 2015-06-15 13:38:40 -0700 2015-06-15 16:06:41 -0700 Patch bug-145986-20150615133805.patch text/plain 2805 mrajca U3VidmVyc2lvbiBSZXZpc2lvbjogMTg1NTU1CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZGY1NTU1ZDEzYTU2NWZl ZWQxZjdmZmY0ZWU0OTBiZDg3ZDJlMzU0OC4uMGUxZDE1ZjFhMWE4NmRkZDRhZmM3ZWI0NDc2Njcy ZGYwMWFlYzViZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDE4IEBACiAyMDE1LTA2LTE1ICBNYXR0 IFJhamNhICA8bXJhamNhQGFwcGxlLmNvbT4KIAorICAgICAgICBNZWRpYSBTZXNzaW9uOiBJbXBy b3ZlIHRoZSBzYWZldHkgb2YgcGxheWJhY2sgdG9nZ2xpbmcKKyAgICAgICAgaHR0cHM6Ly9idWdz LndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0NTk4NgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgICogTW9kdWxlcy9tZWRpYXNlc3Npb24vTWVkaWFT ZXNzaW9uLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6Ok1lZGlhU2Vzc2lvbjo6dG9nZ2xlUGxheWJh Y2spOiBJbXByb3ZlZCB0aGUgc2FmZXR5IG9mIHRoZSBsb29wIHNvIHRoYXQgd2UgZG9uJ3QgcmUt dmlzaXQgZWxlbWVudHMgdGhhdAorICAgICAgICAgIG1heSBoYXZlIGJlZW4gZGVsZXRlZCB1bmRl cm5lYXRoIHVzLgorICAgICAgICAqIE1vZHVsZXMvbWVkaWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5o OiBBZGRlZCBhIHBvaW50ZXIgdG8gdGhlIHNldCBvZiBpdGVyYXRlZCBhY3RpdmUgcGFydGljaXBh dGluZyBlbGVtZW50cyBzbworICAgICAgICAgIHdlIGNhbiByZW1vdmUgYW55IGVsZW1lbnRzIHRo YXQgYXJlIGRlbGV0ZWQgZnJvbSB0aGUgdW5kZXJseWluZyAicmVhbCIgc2V0LgorCisyMDE1LTA2 LTE1ICBNYXR0IFJhamNhICA8bXJhamNhQGFwcGxlLmNvbT4KKwogICAgICAgICBNZWRpYSBTZXNz aW9uOiBBY3RpdmUgcGFydGljaXBhdGluZyBlbGVtZW50cyBjYW4gY2hhbmdlIHdoaWxlIGJlaW5n IGl0ZXJhdGVkIAogICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/ aWQ9MTQ1OTc4CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzZXNz aW9uL01lZGlhU2Vzc2lvbi5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc2Vzc2lv bi9NZWRpYVNlc3Npb24uY3BwCmluZGV4IDQyOThiMjY4YjgyNTc1NmU5MjY4YmY5OTM4ZTc4ZGQx NTA0YmVkNmYuLjJmMjk1YWJhNmU4ZTE3OTRmNjE4ZDhlZGZlYWNhN2Q2ZDQ3MDMzZWYgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvbWVkaWFzZXNzaW9uL01lZGlhU2Vzc2lvbi5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9tZWRpYXNlc3Npb24vTWVkaWFTZXNzaW9u LmNwcApAQCAtOTQsMTQgKzk0LDIxIEBAIGJvb2wgTWVkaWFTZXNzaW9uOjppbnZva2UoKQogCiB2 b2lkIE1lZGlhU2Vzc2lvbjo6dG9nZ2xlUGxheWJhY2soKQogeworICAgIEFTU0VSVCghbV9pdGVy YXRlZEFjdGl2ZVBhcnRpY2lwYXRpbmdFbGVtZW50cyk7CisKICAgICBIYXNoU2V0PEhUTUxNZWRp YUVsZW1lbnQqPiBhY3RpdmVQYXJ0aWNpcGF0aW5nRWxlbWVudHNDb3B5ID0gbV9hY3RpdmVQYXJ0 aWNpcGF0aW5nRWxlbWVudHM7CisgICAgbV9pdGVyYXRlZEFjdGl2ZVBhcnRpY2lwYXRpbmdFbGVt ZW50cyA9ICZhY3RpdmVQYXJ0aWNpcGF0aW5nRWxlbWVudHNDb3B5OworCisgICAgd2hpbGUgKCFh Y3RpdmVQYXJ0aWNpcGF0aW5nRWxlbWVudHNDb3B5LmlzRW1wdHkoKSkgeworICAgICAgICBIVE1M TWVkaWFFbGVtZW50KiBlbGVtZW50ID0gYWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzQ29weS50 YWtlQW55KCk7CiAKLSAgICBmb3IgKGF1dG8qIGVsZW1lbnQgOiBhY3RpdmVQYXJ0aWNpcGF0aW5n RWxlbWVudHNDb3B5KSB7CiAgICAgICAgIGlmIChlbGVtZW50LT5wYXVzZWQoKSkKICAgICAgICAg ICAgIGVsZW1lbnQtPnBsYXkoKTsKICAgICAgICAgZWxzZQogICAgICAgICAgICAgZWxlbWVudC0+ cGF1c2UoKTsKICAgICB9CisKKyAgICBtX2l0ZXJhdGVkQWN0aXZlUGFydGljaXBhdGluZ0VsZW1l bnRzID0gbnVsbHB0cjsKIH0KIAogfQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvTW9kdWxl cy9tZWRpYXNlc3Npb24vTWVkaWFTZXNzaW9uLmggYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21l ZGlhc2Vzc2lvbi9NZWRpYVNlc3Npb24uaAppbmRleCBkODhiODNmNGUzZWQ0NDAzYjFjZGIyZDg4 MDc5YmU1Yzc2MTYwZDdmLi5jNTZhYzk0ZDE0NGFkYTE3NDA0MGFhZjg3NDkwMjM4MTVkMDJjNGU0 IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc2Vzc2lvbi9NZWRpYVNl c3Npb24uaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVzL21lZGlhc2Vzc2lvbi9NZWRpYVNl c3Npb24uaApAQCAtNzMsNiArNzMsNyBAQCBwcml2YXRlOgogICAgIFN0YXRlIG1fY3VycmVudFN0 YXRlIHsgU3RhdGU6OklkbGUgfTsKICAgICBWZWN0b3I8SFRNTE1lZGlhRWxlbWVudCo+IG1fcGFy dGljaXBhdGluZ0VsZW1lbnRzOwogICAgIEhhc2hTZXQ8SFRNTE1lZGlhRWxlbWVudCo+IG1fYWN0 aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzOworICAgIEhhc2hTZXQ8SFRNTE1lZGlhRWxlbWVudCo+ KiBtX2l0ZXJhdGVkQWN0aXZlUGFydGljaXBhdGluZ0VsZW1lbnRzIHsgbnVsbHB0ciB9OwogCiAg ICAgY29uc3QgU3RyaW5nIG1fa2luZDsKICAgICBSZWZQdHI8TWVkaWFSZW1vdGVDb250cm9scz4g bV9jb250cm9sczsK