Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1856373004:
Fix potential null pointer access in HTMLMediaElement::seek (Closed)
|
Created:
4 years, 8 months ago by pavor Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, Ken Russell (switch to Gerrit), mlamouri+watch-blink_chromium.org, philipj_slow, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix potential null pointer access in HTMLMediaElement::seek
Additional check of m_webMediaPlayer for nullptr
in case of a situation when ready state goes out of sync with
m_webMediaPlayer. The real call sequence that results in error is unkown.
BUG=600958
R=liberato@chromium.org,mlamouri@chromium.org
Committed: https://crrev.com/e0dc013c18f0cd16f9cd72004043900152a324eb
Cr-Commit-Position: refs/heads/master@{#396098}
Patch Set 1 #
Total comments: 8
Patch Set 2 : correct review comments #
Total comments: 1
Patch Set 3 : remove HTMLMediaElementTest #Patch Set 4 : Fix potential null pointer in HTMLMediaElement::seek #Messages
Total messages: 29 (10 generated)
Description was changed from ========== crbug-600958: fix potential null pointer access Additional checks of webMediaPlayer() for nullptr and test with manual setup of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,kbr@chromium.org ========== to ========== crbug-600958: fix potential null pointer access Additional checks of webMediaPlayer() for nullptr and test with manual setup of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,kbr@chromium.org ==========
liberato@chromium.org changed reviewers: + philipj@opera.com
(+philipj) thanks -fl https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1692: if (m_readyState == HAVE_NOTHING) would it be better to early out here on !webMediaPlayer()? i'm not sure if the side-effects of setIgnorePreloadNone are intended in this case anyway. i'll defer to kbr and philipj.
https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1692: if (m_readyState == HAVE_NOTHING) On 2016/04/06 14:58:05, liberato wrote: > would it be better to early out here on !webMediaPlayer()? i'm not sure if the > side-effects of setIgnorePreloadNone are intended in this case anyway. > > i'll defer to kbr and philipj. I think so, yes. Also duplicate the FIXME from HTMLMediaElement::duration if you take this route. https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2140: if (webMediaPlayer()) Have you seen a crash here? I'd like to avoid spreading the hack any further than necessary, so the same question applies below too. https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.h:647: friend class HTMLVideoElementTest; This ends up looking strange, can you add a new HTMLMediaElementTest.cpp instead? https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:94: // m_webMediaPlayer. The real call sequence that results in error is unkown. This isn't very reassuring. Can you look at all the places that change readyState and m_webMediaPlayer and list what the remaining candidates are? It could be that nobody has tried very hard to figure this out, at least I haven't.
Also, can you fix the description to not include "crbug-600958" and instead say something about media elements or webMediaPlayer() for context?
I'm not a good reviewer for this change. Attempting to remove myself.
kbr@chromium.org changed reviewers: - kbr@chromium.org
On 2016/04/06 15:08:01, philipj_slow wrote: > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1692: if (m_readyState > == HAVE_NOTHING) > On 2016/04/06 14:58:05, liberato wrote: > > would it be better to early out here on !webMediaPlayer()? i'm not sure if > the > > side-effects of setIgnorePreloadNone are intended in this case anyway. > > > > i'll defer to kbr and philipj. > > I think so, yes. Also duplicate the FIXME from HTMLMediaElement::duration if you > take this route. > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2140: if > (webMediaPlayer()) > Have you seen a crash here? I'd like to avoid spreading the hack any further > than necessary, so the same question applies below too. > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.h (right): > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.h:647: friend class > HTMLVideoElementTest; > This ends up looking strange, can you add a new HTMLMediaElementTest.cpp > instead? > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:94: // > m_webMediaPlayer. The real call sequence that results in error is unkown. > This isn't very reassuring. Can you look at all the places that change > readyState and m_webMediaPlayer and list what the remaining candidates are? It > could be that nobody has tried very hard to figure this out, at least I haven't. I made these changes. I am not really sure about HTMLMediaElementTest, if we require it at all with such a small change in HTMLMediaElement.
https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2140: if (webMediaPlayer()) On 2016/04/06 15:08:01, philipj wrote: > Have you seen a crash here? I'd like to avoid spreading the hack any further > than necessary, so the same question applies below too. I have only seen a crash inside seek. Because I haven't figured out the reason, and there is a lot of discrepancy in the rest of the file whether to check it or not, and even how to access m_webMediaPlayer pointer (directly as a field or with webMediaPlayer()), I decided to leave without checks only WebMediaPlayerClient overrides, which appear to be called from webMediaPlayer itself. https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLVideoElementTest.cpp:94: // m_webMediaPlayer. The real call sequence that results in error is unkown. On 2016/04/06 15:08:01, philipj wrote: > This isn't very reassuring. Can you look at all the places that change > readyState and m_webMediaPlayer and list what the remaining candidates are? It > could be that nobody has tried very hard to figure this out, at least I haven't. I tried to figure it out, but without any success. If there is no possibility of concurrent calls to HTMLMediaElement methods from different threads, I completely have no idea. I consider the candidate for the cause is clearMediaPlayerAndAudioSourceProviderClientWithoutLocking, but I couldn't reproduce the crash and find the scenario.
liberato@chromium.org changed reviewers: + mlamouri@chromium.org
lgtm, though i'm not an owner of HTMLMediaElement. +mlamouri, who is. thanks -fl https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2140: if (webMediaPlayer()) On 2016/05/12 12:03:36, pavor wrote: > On 2016/04/06 15:08:01, philipj wrote: > > Have you seen a crash here? I'd like to avoid spreading the hack any further > > than necessary, so the same question applies below too. > I have only seen a crash inside seek. Because I haven't figured out the reason, > and there is a lot of discrepancy in the rest of the file whether to check it or > not, and even how to access m_webMediaPlayer pointer (directly as a field or > with webMediaPlayer()), I decided to leave without checks only > WebMediaPlayerClient overrides, which appear to be called from webMediaPlayer > itself. do you see these crashes only on android? can you tell if the player is src= or mse? some non-trivial work happens in WebMediaPlayerAndroid's (WMPA) destructor. it notifies |media_source_delegate_| to stop, which in turn does things like talks to the MSE demuxer. If some callback into WMPA happens during that process, such as UpdateReadyState, then i could see WMPA calling back into HTMLMediaElement and resetting the ready state. however, that's all conjecture on my part. i have no idea if those callbacks happen, or if the ready state is reset in HTMLMediaElement before or after the media player is cleared. destroying the weak refs explicitly in WMPA::~WMPA would prevent it, if that's true.
Thanks for the test! Though, I'm worried that a unit test might be biased because it is possible to exercise paths that web content couldn't take. Were you able to write a LayoutTest for this? https://codereview.chromium.org/1856373004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp (right): https://codereview.chromium.org/1856373004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp:34: m_video->remoteRouteAvailabilityChanged(true); Do you really need this line?
On 2016/05/19 13:13:03, Mounir Lamouri wrote: > Thanks for the test! Though, I'm worried that a unit test might be biased > because it is possible to exercise paths that web content couldn't take. Were > you able to write a LayoutTest for this? > > https://codereview.chromium.org/1856373004/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp (right): > I couldn't write layout test that addresses the situation with "broken" state. > https://codereview.chromium.org/1856373004/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/html/HTMLMediaElementTest.cpp:34: > m_video->remoteRouteAvailabilityChanged(true); > Do you really need this line? Seems that it's left from my previous tests for requestRemotePlayback and requestRemotePlaybackControl, don't need it for setCurrentTime
On 2016/05/18 17:27:54, liberato wrote: > lgtm, though i'm not an owner of HTMLMediaElement. > > +mlamouri, who is. > > thanks > -fl > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): > > https://codereview.chromium.org/1856373004/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:2140: if > (webMediaPlayer()) > On 2016/05/12 12:03:36, pavor wrote: > > On 2016/04/06 15:08:01, philipj wrote: > > > Have you seen a crash here? I'd like to avoid spreading the hack any further > > > than necessary, so the same question applies below too. > > I have only seen a crash inside seek. Because I haven't figured out the > reason, > > and there is a lot of discrepancy in the rest of the file whether to check it > or > > not, and even how to access m_webMediaPlayer pointer (directly as a field or > > with webMediaPlayer()), I decided to leave without checks only > > WebMediaPlayerClient overrides, which appear to be called from webMediaPlayer > > itself. > > do you see these crashes only on android? can you tell if the player is src= or > mse? > > some non-trivial work happens in WebMediaPlayerAndroid's (WMPA) destructor. it > notifies |media_source_delegate_| to stop, which in turn does things like talks > to the MSE demuxer. If some callback into WMPA happens during that process, > such as UpdateReadyState, then i could see WMPA calling back into > HTMLMediaElement and resetting the ready state. > > however, that's all conjecture on my part. i have no idea if those callbacks > happen, or if the ready state is reset in HTMLMediaElement before or after the > media player is cleared. > > destroying the weak refs explicitly in WMPA::~WMPA would prevent it, if that's > true. I see the crashes happen only on android. Couldn't see information about player type in crash dump, in fact, only some of the urls that are in crash dumps I checked contain videos, some doesn't contain video players (at least I don't see them, but probably there could be video ads), for example, http://m.dni.ru/economy/2016/5/18/336952.html. Some pages were containing embedded youtube videos. One thing that I can say for sure is that after the fix was deployed, I don't see any crashes with this signature for the latest versions.
I think the unit test is not correct because it is testing things using private methods and even if we end up fixing the underlying cause of the bug, the test might still crash, giving the wrong signal that the bug wasn't fixed. Basically, this unit test checks that the DCHECK actually crashes if hit. I would prefer a LayoutTest but if you can't reproduce the issue in a LayoutTest, I would recommend removing the unit test and land with just the check.
On 2016/05/20 12:48:35, Mounir Lamouri wrote: > I think the unit test is not correct because it is testing things using private > methods and even if we end up fixing the underlying cause of the bug, the test > might still crash, giving the wrong signal that the bug wasn't fixed. Basically, > this unit test checks that the DCHECK actually crashes if hit. > > I would prefer a LayoutTest but if you can't reproduce the issue in a > LayoutTest, I would recommend removing the unit test and land with just the > check. I think so too, but I couldn't make layout test to reproduce. The change is small and lies in private method. I removed HTMLMediaElementTest and left only the change to if condition.
sgtm! Can you fix the CL description: - title should probably be something like "Fix potentionl null pointer access in HTMLMediaElement::seek()." - title + first line should be the same - you might want to remove the crbugxxxx line. - R= has kbr@, not sure why
Description was changed from ========== crbug-600958: fix potential null pointer access Additional checks of webMediaPlayer() for nullptr and test with manual setup of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,kbr@chromium.org ========== to ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ==========
Description was changed from ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ========== to ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ==========
On 2016/05/23 22:57:19, Mounir Lamouri (slow) wrote: > sgtm! > > Can you fix the CL description: > - title should probably be something like "Fix potentionl null pointer access in > HTMLMediaElement::seek()." > - title + first line should be the same > - you might want to remove the crbugxxxx line. > - R= has kbr@, not sure why I updated CL description using git cl description, don't know if it was necessary or it is just enough to edit it through web interface, also I didn't quite understand which title did you mean, so I updated the title of issue also to match "Fix potential null pointer access in HTMLMediaElement::seek".
lgtm, thanks! :)
The CQ bit was checked by pavor@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from liberato@chromium.org Link to the patchset: https://codereview.chromium.org/1856373004/#ps60001 (title: "Fix potential null pointer in HTMLMediaElement::seek")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1856373004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1856373004/60001
Message was sent while issue was closed.
Description was changed from ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ========== to ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org ========== to ========== Fix potential null pointer access in HTMLMediaElement::seek Additional check of m_webMediaPlayer for nullptr in case of a situation when ready state goes out of sync with m_webMediaPlayer. The real call sequence that results in error is unkown. BUG=600958 R=liberato@chromium.org,mlamouri@chromium.org Committed: https://crrev.com/e0dc013c18f0cd16f9cd72004043900152a324eb Cr-Commit-Position: refs/heads/master@{#396098} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e0dc013c18f0cd16f9cd72004043900152a324eb Cr-Commit-Position: refs/heads/master@{#396098} |
