Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
333813004:
Oilpan: Make MediaControllers ExecutionContext a weak pointer. (Closed)
|
Created:
6 years, 6 months ago by zerny-chromium Modified:
6 years, 6 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, philipj_slow, gasubic, fs, eric.carlson_apple.com, feature-media-reviews_chromium.org, dglazkov+blink, nessy, vcarbune.chromium, oilpan-reviews Base URL:
svn://svn.chromium.org/blink/trunk Project:
blink Visibility:
Public. |
DescriptionOilpan: Make MediaControllers ExecutionContext a weak pointer.
Cause of a flaky ASAN use-after-poison in media/video-frame-accurate-seek.html
R=ager@chromium.org, haraken@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=176111
Patch Set 1 #Patch Set 2 : nullptr #
Total comments: 2
Messages
Total messages: 17 (0 generated)
LGTM!
The CQ bit was checked by zerny@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zerny@chromium.org/333813004/1
The CQ bit was checked by zerny@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zerny@chromium.org/333813004/20001
Message was sent while issue was closed.
Change committed as 176111
Message was sent while issue was closed.
https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... Source/core/html/MediaController.h:129: RawPtrWillBeWeakMember<ExecutionContext> m_executionContext; Who uses this m_executionContext? Probably can we just remove it completely?
Message was sent while issue was closed.
https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... File Source/core/html/MediaController.h (right): https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... Source/core/html/MediaController.h:129: RawPtrWillBeWeakMember<ExecutionContext> m_executionContext; On 2014/06/13 13:23:14, haraken wrote: > > Who uses this m_executionContext? Probably can we just remove it completely? It is part of the EventTarget interface so I don't think we can just remove it.
Message was sent while issue was closed.
On 2014/06/16 10:27:21, zerny-chromium wrote: > https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... > File Source/core/html/MediaController.h (right): > > https://codereview.chromium.org/333813004/diff/20001/Source/core/html/MediaCo... > Source/core/html/MediaController.h:129: RawPtrWillBeWeakMember<ExecutionContext> > m_executionContext; > On 2014/06/13 13:23:14, haraken wrote: > > > > Who uses this m_executionContext? Probably can we just remove it completely? > > It is part of the EventTarget interface so I don't think we can just remove it. Makes sense. LGTM.
Message was sent while issue was closed.
Looks like we've picked up some crashes overnight, breaking when marking one of these weak ExecutionContexts -- https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... Not directly due to this CL; trying to determine the cause.
Message was sent while issue was closed.
On 2014/06/18 06:54:27, sof wrote: > Looks like we've picked up some crashes overnight, breaking when marking one of > these weak ExecutionContexts -- > > > https://storage.googleapis.com/chromium-layout-test-archives/WebKit_Win_Oilpa... > > Not directly due to this CL; trying to determine the cause. Interesting, https://chromium.googlesource.com/chromium/blink/+/9e459ed89e74951deb3b797edd... I have as introducing these crashes.
On Wed, Jun 18, 2014 at 11:11 AM, <sigbjornf@opera.com> wrote: > On 2014/06/18 06:54:27, sof wrote: > >> Looks like we've picked up some crashes overnight, breaking when marking >> one >> > of > >> these weak ExecutionContexts -- >> > > > > https://storage.googleapis.com/chromium-layout-test- > archives/WebKit_Win_Oilpan__dbg_/2140/layout-test-results/ > media/media-controller-playback-crash-log.txt > > Not directly due to this CL; trying to determine the cause. >> > > Interesting, > > > https://chromium.googlesource.com/chromium/blink/+/ > 9e459ed89e74951deb3b797edd8732617bd98ae1 > > I have as introducing these crashes. > That is interesting. Do they disappear if you add back the Document.h include? We are relying pretty heavily on traits and that means that we sometimes need to have various types and trait specializations available to do the right thing. I wonder if something like that is tripping us up here. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2014/06/18 09:13:59, Mads Ager (chromium) wrote: > On Wed, Jun 18, 2014 at 11:11 AM, <mailto:sigbjornf@opera.com> wrote: > > > On 2014/06/18 06:54:27, sof wrote: > > > >> Looks like we've picked up some crashes overnight, breaking when marking > >> one > >> > > of > > > >> these weak ExecutionContexts -- > >> > > > > > > > > https://storage.googleapis.com/chromium-layout-test- > > archives/WebKit_Win_Oilpan__dbg_/2140/layout-test-results/ > > media/media-controller-playback-crash-log.txt > > > > Not directly due to this CL; trying to determine the cause. > >> > > > > Interesting, > > > > > > https://chromium.googlesource.com/chromium/blink/+/ > > 9e459ed89e74951deb3b797edd8732617bd98ae1 > > > > I have as introducing these crashes. > > > > That is interesting. Do they disappear if you add back the Document.h > include? We are relying pretty heavily on traits and that means that we > sometimes need to have various types and trait specializations available to > do the right thing. I wonder if something like that is tripping us up here. > Yes, it does disappear. Thought first it was an unexpected interaction with a recent HTMLMediaElement::executionContext() change somehow, but no. I will try to (slowly) compile down the cause of that, but the rule of not relying on Handle.h to somehow be in scope perhaps needs re-consideration?
Message was sent while issue was closed.
On 2014/06/18 09:48:10, sof wrote: > On 2014/06/18 09:13:59, Mads Ager (chromium) wrote: > > On Wed, Jun 18, 2014 at 11:11 AM, <mailto:sigbjornf@opera.com> wrote: > > > > > On 2014/06/18 06:54:27, sof wrote: > > > > > >> Looks like we've picked up some crashes overnight, breaking when marking > > >> one > > >> > > > of > > > > > >> these weak ExecutionContexts -- > > >> > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test- > > > archives/WebKit_Win_Oilpan__dbg_/2140/layout-test-results/ > > > media/media-controller-playback-crash-log.txt > > > > > > Not directly due to this CL; trying to determine the cause. > > >> > > > > > > Interesting, > > > > > > > > > https://chromium.googlesource.com/chromium/blink/+/ > > > 9e459ed89e74951deb3b797edd8732617bd98ae1 > > > > > > I have as introducing these crashes. > > > > > > > That is interesting. Do they disappear if you add back the Document.h > > include? We are relying pretty heavily on traits and that means that we > > sometimes need to have various types and trait specializations available to > > do the right thing. I wonder if something like that is tripping us up here. > > > > Yes, it does disappear. Thought first it was an unexpected interaction with a > recent HTMLMediaElement::executionContext() change somehow, but no. > > I will try to (slowly) compile down the cause of that, but the rule of not > relying on Handle.h to somehow be in scope perhaps needs re-consideration? Update: Element.h needs to #include ExecutionContext.h (I suspect it is Node.h that needs it, but not verified.) Exact reasons why remains unknown.
Message was sent while issue was closed.
On 2014/06/18 12:31:03, sof wrote: > On 2014/06/18 09:48:10, sof wrote: > > On 2014/06/18 09:13:59, Mads Ager (chromium) wrote: > > > On Wed, Jun 18, 2014 at 11:11 AM, <mailto:sigbjornf@opera.com> wrote: > > > > > > > On 2014/06/18 06:54:27, sof wrote: > > > > > > > >> Looks like we've picked up some crashes overnight, breaking when marking > > > >> one > > > >> > > > > of > > > > > > > >> these weak ExecutionContexts -- > > > >> > > > > > > > > > > > > > > > > https://storage.googleapis.com/chromium-layout-test- > > > > archives/WebKit_Win_Oilpan__dbg_/2140/layout-test-results/ > > > > media/media-controller-playback-crash-log.txt > > > > > > > > Not directly due to this CL; trying to determine the cause. > > > >> > > > > > > > > Interesting, > > > > > > > > > > > > https://chromium.googlesource.com/chromium/blink/+/ > > > > 9e459ed89e74951deb3b797edd8732617bd98ae1 > > > > > > > > I have as introducing these crashes. > > > > > > > > > > That is interesting. Do they disappear if you add back the Document.h > > > include? We are relying pretty heavily on traits and that means that we > > > sometimes need to have various types and trait specializations available to > > > do the right thing. I wonder if something like that is tripping us up here. > > > > > > > Yes, it does disappear. Thought first it was an unexpected interaction with a > > recent HTMLMediaElement::executionContext() change somehow, but no. > > > > I will try to (slowly) compile down the cause of that, but the rule of not > > relying on Handle.h to somehow be in scope perhaps needs re-consideration? > > Update: Element.h needs to #include ExecutionContext.h (I suspect it is Node.h > that needs it, but not verified.) Exact reasons why remains unknown. ( https://codereview.chromium.org/344443008/ handles this. ) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
