168404 2017-02-15 19:48:27 -0800 REGRESSION (r212311): NULL-dereference in HTMLMediaElement::prepareToPlay() 2017-02-21 17:39:14 -0800 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 jer.noble jer.noble ap bweinstein commit-queue eric.carlson simon.fraser webkit-bug-importer oldest_to_newest 1277462 0 jer.noble 2017-02-15 19:48:27 -0800 REGRESSION (r212311): NULL-dereference in HTMLMediaElement::prepareToPlay() 1277463 1 301691 jer.noble 2017-02-15 19:52:52 -0800 Created attachment 301691 Patch 1277464 2 jer.noble 2017-02-15 19:53:20 -0800 rdar://problem/30547188 1277467 3 301691 bweinstein 2017-02-15 19:59:03 -0800 Comment on attachment 301691 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review > Source/WebCore/ChangeLog:10 > + prepareToPlay(). r212311 began calling prepareToPlay() on a subsequent run-loop iteration Extra space between sentences. 1277468 4 301691 bweinstein 2017-02-15 19:59:25 -0800 Comment on attachment 301691 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review >> Source/WebCore/ChangeLog:10 >> + prepareToPlay(). r212311 began calling prepareToPlay() on a subsequent run-loop iteration > > Extra space between sentences. Extra space between sentences. 1277481 5 jer.noble 2017-02-15 20:27:13 -0800 Committed r212420: <http://trac.webkit.org/changeset/212420> 1278116 6 301691 ap 2017-02-16 21:43:28 -0800 Comment on attachment 301691 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=301691&action=review > Source/WebCore/html/HTMLMediaElement.cpp:2562 > m_havePreparedToPlay = true; > - m_player->prepareToPlay(); > + if (m_player) > + m_player->prepareToPlay(); Shouldn't setting m_havePreparedToPlay be inside the "if" too? This looks like it fixes the crash, but keeps the incorrect behavior. 1278119 7 jer.noble 2017-02-16 22:03:52 -0800 (In reply to comment #6) > Comment on attachment 301691 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=301691&action=review > > > Source/WebCore/html/HTMLMediaElement.cpp:2562 > > m_havePreparedToPlay = true; > > - m_player->prepareToPlay(); > > + if (m_player) > > + m_player->prepareToPlay(); > > Shouldn't setting m_havePreparedToPlay be inside the "if" too? > > This looks like it fixes the crash, but keeps the incorrect behavior. No. The behavior is correct. If m_havePreparedToPlay is set, we will call m_player->prepareToPlay() next time the MediaPlayer is created. 1278123 8 jer.noble 2017-02-16 22:10:40 -0800 (In reply to comment #7) > (In reply to comment #6) > > Comment on attachment 301691 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=301691&action=review > > > > > Source/WebCore/html/HTMLMediaElement.cpp:2562 > > > m_havePreparedToPlay = true; > > > - m_player->prepareToPlay(); > > > + if (m_player) > > > + m_player->prepareToPlay(); > > > > Shouldn't setting m_havePreparedToPlay be inside the "if" too? > > > > This looks like it fixes the crash, but keeps the incorrect behavior. > > No. The behavior is correct. If m_havePreparedToPlay is set, we will call > m_player->prepareToPlay() next time the MediaPlayer is created. Double-checking; it's actually the opposite. m_havePreparedToPlay is cleared whenever a MediaPlayer is created. Same deal though: the behavior is exactly the same whether setting m_havePreparedToPlay is inside the null check our outside. 1278124 9 ap 2017-02-16 22:15:47 -0800 Hmm. If the behaviour is correct, then the naming is not. The code says "have prepared to play" without doing the "prepare to play" now. It used to be straightforward before the fix, but now it immediately raises a red flag. 1279688 10 simon.fraser 2017-02-21 17:35:20 -0800 Is this exercised by tests? 1279691 11 jer.noble 2017-02-21 17:39:14 -0800 (In reply to comment #10) > Is this exercised by tests? This code is hit by every autoplay test. 301691 2017-02-15 19:52:52 -0800 2017-02-15 20:25:34 -0800 Patch bug-168404-20170215195016.patch text/plain 1595 jer.noble U3VidmVyc2lvbiBSZXZpc2lvbjogMjEyMzkzCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNmIwYjM2YmI2YTRiMWIx NTBlYjczYmQwYTdhNzc3YTJjODQ3OWNjOS4uYmM2OGQxMTBiYTk0MDIxZTRjMjUyMjQ2ZjQyOWQ2 OTdiZDMxMTRmZiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE4IEBACisyMDE3LTAyLTE1ICBKZXIg Tm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgorCisgICAgICAgIFJFR1JFU1NJT04gKHIyMTIz MTEpOiBOVUxMLWRlcmVmZXJlbmNlIGluIEhUTUxNZWRpYUVsZW1lbnQ6OnByZXBhcmVUb1BsYXko KQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTY4NDA0 CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zMDU0NzE4OD4KKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBQcmlvciB0byByMjEyMzExLCBtX3BsYXllciB3 YXMgYWx3YXlzIGd1YXJhbnRlZWQgdG8gYmUgaW5pdGlhbGl6ZWQgd2hlbiBjYWxsaW5nCisgICAg ICAgIHByZXBhcmVUb1BsYXkoKS4gIHIyMTIzMTEgYmVnYW4gY2FsbGluZyBwcmVwYXJlVG9QbGF5 KCkgb24gYSBzdWJzZXF1ZW50IHJ1bi1sb29wIGl0ZXJhdGlvbgorICAgICAgICBhZnRlciBjcmVh dGluZyBtX3BsYXllci4gU28gbm93IGNoZWNrIHdoZXRoZXIgbV9wbGF5ZXIgaXMgTlVMTCBiZWZv cmUgY2FsbGluZyBtZXRob2RzIG9uIGl0LgorCisgICAgICAgICogaHRtbC9IVE1MTWVkaWFFbGVt ZW50LmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OkhUTUxNZWRpYUVsZW1lbnQ6OnByZXBhcmVUb1Bs YXkpOgorCiAyMDE3LTAyLTEzICBKZXIgTm9ibGUgIDxqZXIubm9ibGVAYXBwbGUuY29tPgogCiAg ICAgICAgIERpc2FibGVkIE1lZGlhIFNvdXJjZXMgc2hvdWxkIHJlbmRlciBibGFjay9zaWxlbmNl CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRpYUVsZW1lbnQuY3BwIGIv U291cmNlL1dlYkNvcmUvaHRtbC9IVE1MTWVkaWFFbGVtZW50LmNwcAppbmRleCBhM2UxZjdmNWZi MmY2MDVhNTY4NDczYTA5NGM2OGEzM2Q3NTk4NTY3Li43ZmEwMTZkZGZlOGY5YjQ2NTUxZGM0YWZh YjUyMDQ0YjU1YTgwNDc5IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9odG1sL0hUTUxNZWRp YUVsZW1lbnQuY3BwCisrKyBiL1NvdXJjZS9XZWJDb3JlL2h0bWwvSFRNTE1lZGlhRWxlbWVudC5j cHAKQEAgLTI1NTgsNyArMjU1OCw4IEBAIHZvaWQgSFRNTE1lZGlhRWxlbWVudDo6cHJlcGFyZVRv UGxheSgpCiAgICAgaWYgKG1faGF2ZVByZXBhcmVkVG9QbGF5KQogICAgICAgICByZXR1cm47CiAg ICAgbV9oYXZlUHJlcGFyZWRUb1BsYXkgPSB0cnVlOwotICAgIG1fcGxheWVyLT5wcmVwYXJlVG9Q bGF5KCk7CisgICAgaWYgKG1fcGxheWVyKQorICAgICAgICBtX3BsYXllci0+cHJlcGFyZVRvUGxh eSgpOwogfQogCiB2b2lkIEhUTUxNZWRpYUVsZW1lbnQ6OmZhc3RTZWVrKGRvdWJsZSB0aW1lKQo=