Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(2)

Issue 1780163002: Revert of IntersectionObserver: use an idle callback to send notifications. (Closed)

Created:
4 years, 9 months ago by sof
Modified:
4 years, 9 months ago
Reviewers:
haraken, ojan, szager1, Sami
CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Revert of IntersectionObserver: use an idle callback to send notifications. (patchset #6 id:100001 of https://codereview.chromium.org/1776493002/ ) Reason for revert: Tests added are leaking, https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20Leak/builds/18011 Original issue's description: > IntersectionObserver: use an idle callback to send notifications. > > With this change, the tests can no longer use setTimeout(0) to wait > for notifications to be delivered. Instead, use takeRecords() to > proactively grab notifications right after they are generated > (typically in a RAF right after a layout change). > > BUG=540528 > R=ojan@chromium.org,haraken@chromium.org > > Committed: https://crrev.com/2c168f38b5c0e4e50374be4e54c44901c60738a9 > Cr-Commit-Position: refs/heads/master@{#380278} TBR=ojan@chromium.org,haraken@chromium.org,skyostil@chromium.org,szager@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG=540528 Committed: https://crrev.com/57644d439ce9bfee85efedff9f27a31e3e0bd3f5 Cr-Commit-Position: refs/heads/master@{#380375}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+693 lines, -751 lines) Patch
M third_party/WebKit/LayoutTests/http/tests/intersection-observer/iframe-cross-origin.html View 1 chunk +2 lines, -6 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/cross-origin-subframe.html View 1 chunk +41 lines, -35 lines 0 comments Download
D third_party/WebKit/LayoutTests/intersection-observer/README View 1 chunk +0 lines, -29 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/containing-block.html View 1 chunk +69 lines, -67 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/iframe-no-root.html View 1 chunk +66 lines, -61 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/iframe-no-root-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/multiple-thresholds.html View 1 chunk +171 lines, -179 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/multiple-thresholds-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/observer-without-js-reference.html View 1 chunk +15 lines, -22 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/root-margin.html View 1 chunk +70 lines, -71 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/root-margin-expected.txt View 1 chunk +4 lines, -6 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/same-document-no-root.html View 1 chunk +57 lines, -56 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/same-document-root.html View 1 chunk +90 lines, -92 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/same-document-root-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/same-document-zero-size-target.html View 1 chunk +58 lines, -56 lines 0 comments Download
M third_party/WebKit/LayoutTests/intersection-observer/timestamp.html View 3 chunks +31 lines, -34 lines 0 comments Download
M third_party/WebKit/Source/core/dom/IdleRequestCallback.h View 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/IntersectionObserverController.h View 3 chunks +5 lines, -6 lines 0 comments Download
M third_party/WebKit/Source/core/dom/IntersectionObserverController.cpp View 4 chunks +14 lines, -27 lines 0 comments Download

Messages

Total messages: 9 (1 generated)
sof
Created Revert of IntersectionObserver: use an idle callback to send notifications.
4 years, 9 months ago (2016-03-10 08:38:55 UTC) #1
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1780163002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1780163002/1
4 years, 9 months ago (2016-03-10 08:39:11 UTC) #2
commit-bot: I haz the power
Committed patchset #1 (id:1)
4 years, 9 months ago (2016-03-10 08:40:35 UTC) #3
commit-bot: I haz the power
Patchset 1 (id:??) landed as https://crrev.com/57644d439ce9bfee85efedff9f27a31e3e0bd3f5 Cr-Commit-Position: refs/heads/master@{#380375}
4 years, 9 months ago (2016-03-10 08:41:44 UTC) #5
szager1
What does this mean? The leak log is pretty light on details.
4 years, 9 months ago (2016-03-10 08:43:44 UTC) #6
sof
On 2016/03/10 08:43:44, szager1 wrote: > What does this mean? The leak log is pretty ...
4 years, 9 months ago (2016-03-10 09:38:36 UTC) #7
haraken
LGTM
4 years, 9 months ago (2016-03-10 10:09:13 UTC) #8
szager1
4 years, 9 months ago (2016-03-12 02:06:15 UTC) #9
Message was sent while issue was closed.
On 2016/03/10 09:38:36, sof wrote:
> On 2016/03/10 08:43:44, szager1 wrote:
> > What does this mean?  The leak log is pretty light on details.
> 
> A document is being indirectly kept alive from a posted idle callback on
> shutdown, I'm guessing.
> 
> Run layout tests with --enable-leak-detection to reproduce & debug.

Running with --enable-leak-detection gives me:

[1/1] inters...ot.html failed unexpectedly (leak detected:
({"numberOfLiveActiveDOMObjects":[2,3]}))

I modified the test code to pause long enough to let the idle task complete
before finishing, and with some printf debugging, I have verified that there's
no unfinished IntersectionObserverController idle task at shutdown.

So I'm at a loss.  How do I debug this further?

Powered by Google App Engine
This is Rietveld 408576698