Commit 60944f94 authored by Olga Gerchikov's avatar Olga Gerchikov Committed by Commit Bot

Handle instantiation of scroll linked animations with uninitialized scroll source.

This changes fixes Issue 976633: Null-dereference READ in blink::ScrollTimeline::GetDocument.

I can't recreate the crash scenario, but I suspect that it's a racing condition when
ScrollTimeline is created with uninitialized document.scrollingElement.

The fix is to initialize ScrollTimeline.document with the document passed in the constructor
instead of consulting the scroll source.

Bug: 976633
Change-Id: I76a7b660e543e3a0ea9512554bb194813a203a2f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1670074Reviewed-by: default avatarRobert Flack <flackr@chromium.org>
Commit-Queue: Olga Gerchikov <gerchiko@microsoft.com>
Cr-Commit-Position: refs/heads/master@{#671457}
parent 89cad226
...@@ -109,18 +109,20 @@ ScrollTimeline* ScrollTimeline::Create(Document& document, ...@@ -109,18 +109,20 @@ ScrollTimeline* ScrollTimeline::Create(Document& document,
} }
return MakeGarbageCollected<ScrollTimeline>( return MakeGarbageCollected<ScrollTimeline>(
scroll_source, orientation, start_scroll_offset, end_scroll_offset, &document, scroll_source, orientation, start_scroll_offset,
options->timeRange().GetAsDouble(), end_scroll_offset, options->timeRange().GetAsDouble(),
Timing::StringToFillMode(options->fill())); Timing::StringToFillMode(options->fill()));
} }
ScrollTimeline::ScrollTimeline(Element* scroll_source, ScrollTimeline::ScrollTimeline(Document* document,
Element* scroll_source,
ScrollDirection orientation, ScrollDirection orientation,
CSSPrimitiveValue* start_scroll_offset, CSSPrimitiveValue* start_scroll_offset,
CSSPrimitiveValue* end_scroll_offset, CSSPrimitiveValue* end_scroll_offset,
double time_range, double time_range,
Timing::FillMode fill) Timing::FillMode fill)
: scroll_source_(scroll_source), : document_(document),
scroll_source_(scroll_source),
resolved_scroll_source_(ResolveScrollSource(scroll_source_)), resolved_scroll_source_(ResolveScrollSource(scroll_source_)),
orientation_(orientation), orientation_(orientation),
start_scroll_offset_(start_scroll_offset), start_scroll_offset_(start_scroll_offset),
...@@ -348,6 +350,7 @@ void ScrollTimeline::AnimationDetached(Animation*) { ...@@ -348,6 +350,7 @@ void ScrollTimeline::AnimationDetached(Animation*) {
} }
void ScrollTimeline::Trace(blink::Visitor* visitor) { void ScrollTimeline::Trace(blink::Visitor* visitor) {
visitor->Trace(document_);
visitor->Trace(scroll_source_); visitor->Trace(scroll_source_);
visitor->Trace(resolved_scroll_source_); visitor->Trace(resolved_scroll_source_);
visitor->Trace(start_scroll_offset_); visitor->Trace(start_scroll_offset_);
......
...@@ -40,7 +40,8 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline { ...@@ -40,7 +40,8 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
ScrollTimelineOptions*, ScrollTimelineOptions*,
ExceptionState&); ExceptionState&);
ScrollTimeline(Element*, ScrollTimeline(Document*,
Element*,
ScrollDirection, ScrollDirection,
CSSPrimitiveValue*, CSSPrimitiveValue*,
CSSPrimitiveValue*, CSSPrimitiveValue*,
...@@ -50,7 +51,7 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline { ...@@ -50,7 +51,7 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
// AnimationTimeline implementation. // AnimationTimeline implementation.
double currentTime(bool& is_null) final; double currentTime(bool& is_null) final;
bool IsScrollTimeline() const override { return true; } bool IsScrollTimeline() const override { return true; }
Document* GetDocument() override { return &scroll_source_->GetDocument(); } Document* GetDocument() override { return document_; }
// ScrollTimeline is not active if scrollSource is null, does not currently // ScrollTimeline is not active if scrollSource is null, does not currently
// have a CSS layout box, or if its layout box is not a scroll container. // have a CSS layout box, or if its layout box is not a scroll container.
// https://github.com/WICG/scroll-animations/issues/31 // https://github.com/WICG/scroll-animations/issues/31
...@@ -96,6 +97,7 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline { ...@@ -96,6 +97,7 @@ class CORE_EXPORT ScrollTimeline final : public AnimationTimeline {
static bool HasActiveScrollTimeline(Node* node); static bool HasActiveScrollTimeline(Node* node);
private: private:
Member<Document> document_;
// Use |scroll_source_| only to implement the web-exposed API but use // Use |scroll_source_| only to implement the web-exposed API but use
// resolved_scroll_source_ to actually access the scroll related properties. // resolved_scroll_source_ to actually access the scroll related properties.
Member<Element> scroll_source_; Member<Element> scroll_source_;
......
...@@ -231,7 +231,7 @@ TEST_F(ScrollTimelineTest, AttachOrDetachAnimationWithNullScrollSource) { ...@@ -231,7 +231,7 @@ TEST_F(ScrollTimelineTest, AttachOrDetachAnimationWithNullScrollSource) {
CSSPrimitiveValue* start_scroll_offset = nullptr; CSSPrimitiveValue* start_scroll_offset = nullptr;
CSSPrimitiveValue* end_scroll_offset = nullptr; CSSPrimitiveValue* end_scroll_offset = nullptr;
ScrollTimeline* scroll_timeline = MakeGarbageCollected<ScrollTimeline>( ScrollTimeline* scroll_timeline = MakeGarbageCollected<ScrollTimeline>(
scroll_source, ScrollTimeline::Block, start_scroll_offset, &GetDocument(), scroll_source, ScrollTimeline::Block, start_scroll_offset,
end_scroll_offset, 100, Timing::FillMode::NONE); end_scroll_offset, 100, Timing::FillMode::NONE);
// Sanity checks. // Sanity checks.
......
...@@ -82,7 +82,7 @@ TEST_F(ScrollTimelineUtilTest, ToCompositorScrollTimelineNullScrollSource) { ...@@ -82,7 +82,7 @@ TEST_F(ScrollTimelineUtilTest, ToCompositorScrollTimelineNullScrollSource) {
CSSPrimitiveValue* start_scroll_offset = nullptr; CSSPrimitiveValue* start_scroll_offset = nullptr;
CSSPrimitiveValue* end_scroll_offset = nullptr; CSSPrimitiveValue* end_scroll_offset = nullptr;
ScrollTimeline* timeline = MakeGarbageCollected<ScrollTimeline>( ScrollTimeline* timeline = MakeGarbageCollected<ScrollTimeline>(
scroll_source, ScrollTimeline::Block, start_scroll_offset, &GetDocument(), scroll_source, ScrollTimeline::Block, start_scroll_offset,
end_scroll_offset, 100, Timing::FillMode::NONE); end_scroll_offset, 100, Timing::FillMode::NONE);
std::unique_ptr<CompositorScrollTimeline> compositor_timeline = std::unique_ptr<CompositorScrollTimeline> compositor_timeline =
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment