🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168

torch2424 · 2018-11-07T00:37:06Z

continuation of #19034

This makes the Mutation observer use getBoundingClientRect() to get around the resource scheduler not handling amp elements and updating their Layout Box from getLayoutBox() in time before the mutation pass is called to detect the changes.

cc @jridgewell Just wanted to make you aware of this. If you know of a better way to handle this please let us know. This is the best solution @zhouyx , @lannka , and I could come up with for this specific situation.

Example

This example was pulled by recreating the Fandango site by hand from the output AMP and "decompiling" it.

…image element is created.

zhouyx · 2018-11-07T18:18:01Z

src/intersection-observer-polyfill.js

+    // Than our layout box, in order to avoid race conditions 
+    // with the resource scheduler
+    if (opt_calledFromMutationObserver) {
+      elementRect = element./*OK*/getBoundingClientRect();


@torch2424 elementRect is the rect relative to the document not viewport. We can't use getBoundingClientRect() directly.

torch2424 · 2018-11-07T22:00:13Z

Made requested changes, this PR is good to go 😄

zhouyx · 2018-11-07T23:29:49Z

@jridgewell would really like to get your feedback on this.

The reason why we need to call Viewport.getLayoutRect() here is because getLayoutBox() hasn't been updated when mutationObserver fire. I know that you want to only observe the document hidden attribute for performance reason, but calling getBoundingClientRect is expensive which I want to avoid.

Do you think it's possible to have mutationObserver only observe the AMP element's attribute change that is selected by visibility trigger. So that in this example InOb will use the <amp-img> layoutBox whenever it has been laid out. However I am not sure how to detect when [hidden] is added to the container. Do you know a good way to detect that.

zhouyx · 2018-11-07T23:30:50Z

src/intersection-observer-polyfill.js

+    // Than our layout box, in order to avoid race conditions
+    // with the resource scheduler
+    if (opt_calledFromMutationObserver) {
+      elementRect = this.viewport_.getLayoutRect(element);


@jridgewell Do we need different method when layer is enabled?

No, layers is corrected implemented in getLayoutRect. Note it returns a viewport relative box (not a page relative box), but that's no different than the current code.

jridgewell · 2018-11-07T23:53:26Z

src/intersection-observer-polyfill.js

+    // Than our layout box, in order to avoid race conditions
+    // with the resource scheduler
+    if (opt_calledFromMutationObserver) {
+      elementRect = this.viewport_.getLayoutRect(element);


No, layers is corrected implemented in getLayoutRect. Note it returns a viewport relative box (not a page relative box), but that's no different than the current code.

jridgewell · 2018-11-07T23:54:26Z

src/intersection-observer-polyfill.js

+    // with the resource scheduler
+    if (opt_calledFromMutationObserver) {
+      elementRect = this.viewport_.getLayoutRect(element);
+      ownerRect = owner && this.viewport_.getLayoutRect(owner);


These need to be inside measure phases.

jridgewell · 2018-11-07T23:55:22Z

src/intersection-observer-polyfill.js

+   *    Whether this was called from a mutation observer
+   *    This will signal that the layout of the elements
+   *    must use getBoundingClientRect, as Resource Scheduler
+   *    could be called too late, and not clear the layout cache.


You could instead trigger tick on the next Resource pass, which will mean everything's been remeasured. Then we don't need booleans floating around.

Oh nice! Yeah let's try that, I think that would be the most optimal solution. Are the Resource passes usually done fast? As in there is nothing that could make it take longer than a second?

And what is the function/service to wait for this?

Are the Resource passes usually done fast? As in there is nothing that could make it take longer than a second?

They can take up to 30s if there's no activity on the page (or it's in the background). But since there was a mutation, it'll happen in the next 100ms.

And what is the function/service to wait for this?

There's not currently one to do this. We can add a method to Resources, say onNextPass. Then, at the end of Resources.p.doPass, we resolve call all the pending callbacks, and clear out the array.

class Resources { constructor() { // ... /** @const @private {!Array<function()>} */ this.onNextPass_ = []; } doPass() { //... for (let i = 0; i < this.onNextPass_.length; i++) { const fn = this.onNextPass_[i]; fn(); } this.onNextPass_.length = 0; } /** * Registers a callback to be called when the next pass happens. * @param {function()} */ onNextPass(fn) { this.onNextPass_.push(fn); } }

torch2424 · 2018-11-10T02:51:36Z

cc @jridgewell @zhouyx

Made all requested changes, using the suggested onNextPass. Also, added tests for this new feature on Resources.

Lastly, confirmed that this solution works on my example! 🎉

Thank you everyone for the help! 😄

torch2424 · 2018-11-12T19:11:24Z

@jridgewell @zhouyx

@aghassemi Just opened a P1 at #19252

Would like to get this pulled in so we can make a fix for this.

Will fix the remaining travis tests once I am sure this is the way we want to go. Thanks! 😄

jridgewell · 2018-11-12T22:38:42Z

src/intersection-observer-polyfill.js

-          this.tick(this.viewport_.getRect(), undefined, true);
-        }
-      });
+      this.resources_ = Services.resourcesForDoc(ampdoc);


There's no need to set and unset this. Just store it as a const reference.

I can keep the reference, however, I can't store it as a constant, as we do a check for if the window has MutationObserver before attempting to bind everything.

There's no need to gate the resources reference on whether the browser supports MutationObserver.

Ah yes! Good Point 😄 But another thing is, we don't have access to the ampdoc, until we get our first element observe. As the constructor of the polyfill only takes in a callback. 😮

jridgewell · 2018-11-12T22:39:56Z

src/intersection-observer-polyfill.js

-      return;
+    if (this.viewport_ && this.resources_) {
+      this.resources_.onNextPass(() => {
+        this.tick(this.viewport_.getRect(), undefined, true);


This can cause multiple ticks to happen back to back. If tick is not already throttled, it should be.

jridgewell · 2018-11-12T22:40:54Z

src/service/resources-impl.js

@@ -1201,11 +1212,16 @@ export class Resources {
          this.viewer_.sendMessage('documentHeight',
              dict({'height': measuredContentHeight}), /* cancelUnsent */true);
          this.contentHeight_ = measuredContentHeight;
-          dev().fine(TAG_, 'document height changed: ' + this.contentHeight_);
+          dev().fine(TAG_, 'document height changed: %s', this.contentHeight_);


jridgewell · 2018-11-12T22:42:13Z

src/service/resources-impl.js

          this.viewport_.contentHeightChanged();
        }
      });
    }
+
+    this.passCallbacks_.forEach(callback => {


Runtime has a different set of performance requirements. Please use a for-loop here.

Also when you fix this, don't do passCallbacks[i]() because that'll leak passCallbacks as the this context of the function invocation. Store fn = passCallbacks[i], then invoke fn().

jridgewell · 2018-11-12T22:43:36Z

test/functional/test-resources.js

@@ -1634,6 +1634,34 @@ describe('Resources discoverWork', () => {
      });
    });
  });
+
+  describe('passCallbacks_', () => {


"onNextPass"

jridgewell · 2018-11-12T22:44:21Z

test/functional/test-resources.js

+      resources.onNextPass(passCallback);
+      resources.doPass();
+
+      expect(resources.passCallbacks_.length).to.be.equal(0);


Let's not write tests on passCallbacks_, since it's private. The intention here is that the fn isn't called twice, so let's run two passes and assert the fn isn't called twice.

torch2424 · 2018-11-13T19:50:40Z

@zhouyx @jridgewell

Made requested changes. This is good to go 😄

jridgewell · 2018-11-13T20:12:45Z

src/intersection-observer-polyfill.js

+   * @private
+   */
+  handleMutationObserverPass_() {
+    if (this.viewport_ && this.resources_) {


No need to check this.

jridgewell · 2018-11-13T20:12:53Z

src/intersection-observer-polyfill.js

+        this.tick(this.viewport_.getRect());
+      });
+    }
+    return;


Useless return.

src/intersection-observer-polyfill.js

jridgewell · 2018-11-13T20:14:08Z

src/intersection-observer-polyfill.js

+  handleMutationObserverPass_() {
+    if (this.viewport_ && this.resources_) {
+      this.resources_.onNextPass(() => {
+        this.tick(this.viewport_.getRect());


tick still needs to be throttled.

Tick is being throttled by the pass isn't it? 😮 Or do you want the pass inside the tick itself?

Oh, I totally forgot we were using a Pass. This is fine, then.

torch2424 · 2018-11-13T22:40:42Z

@jridgewell @zhouyx

Replied to comments, and made more requested changes 😄

This is good to go.

zhouyx · 2018-11-14T17:18:09Z

src/service/resources-impl.js

+      const fn = this.passCallbacks_[i];
+      fn();
+    }
+    this.passCallbacks_.length = 0;


Ha! Didn't know we can set length to 0 to clean an array : )

Gotta give the credit to @jridgewell on that one, I didn't know either 😂

zhouyx · 2018-11-14T17:20:54Z

test/functional/test-resources.js

+
+      const passCallback = sandbox.spy();
+      resources.onNextPass(passCallback);
+      resources.doPass();


nit: if I understand it correctly, the callback will be called the first doPass() is called. would be great to add that check as well with expect(passCallback).to.be.calledOnce;

zhouyx · 2018-11-14T17:24:07Z

src/intersection-observer-polyfill.js

-        }
-      });
+
+      if (!this.viewport_) {


Is this.viewport_ and this.resources_ used elsewhere? And can element and ampdoc be passed to the handleMutationObserverPass? Asking because would prefer use calling Services.viewportForDoc(element) and Services.resourcesForDoc(ampdoc) directly instead. The cost difference should be minimum.

torch2424 · 2018-11-14T20:48:58Z

@jridgewell @zhouyx

Made requested changes 😄

This is good to go.

zhouyx

Thank you for the fix!!

…ndango-bug-fix-no-example

torch2424 · 2018-11-15T00:46:19Z

Thank you! @zhouyx

Whenever you get the time @jridgewell let me know what you think and we can merge this in 😄

…ion (ampproject#19168) * Got a working half-broken page that we can test with * COnfirmed the bug is that the pass isn't long enough * Going to go home. But I learned that it doesn't fire until the child image element is created. * I was doing something * Fixed the polyfill completely * Fixed linting * Fixed all precommit checks * Removed the one-off fandango example * Made PR Changes * Created an onNextPass for resources * Fixed tests for ampdoc * Removed the original getLayoutRect * Removed a dangling opt_calledFromMutationObserver * Made PR Changes * Fixed refresh tests for intersection observer * Removed returns * Fixed some linting errors * Made PR Changes * Fixed linting errors

torch2424 added 8 commits November 5, 2018 17:30

Got a working half-broken page that we can test with

7ef9930

COnfirmed the bug is that the pass isn't long enough

ad491aa

Going to go home. But I learned that it doesn't fire until the child …

63caf4b

…image element is created.

I was doing something

2be1f3b

Fixed the polyfill completely

59f561e

Fixed linting

c17cb5c

Fixed all precommit checks

87ec925

Removed the one-off fandango example

f96fd1b

torch2424 assigned zhouyx and lannka Nov 7, 2018

torch2424 requested review from jridgewell, zhouyx and lannka November 7, 2018 00:37

googlebot added the cla: yes label Nov 7, 2018

zhouyx suggested changes Nov 7, 2018

View reviewed changes

Made PR Changes

0eba342

zhouyx reviewed Nov 7, 2018

View reviewed changes

jridgewell requested changes Nov 7, 2018

View reviewed changes

lannka removed their request for review November 9, 2018 22:55

lannka unassigned zhouyx and lannka Nov 9, 2018

torch2424 added 4 commits November 9, 2018 18:26

Created an onNextPass for resources

d5051b9

Fixed tests for ampdoc

9892c8e

Removed the original getLayoutRect

42a9964

Removed a dangling opt_calledFromMutationObserver

c7282f8

jridgewell reviewed Nov 12, 2018

View reviewed changes

torch2424 added 2 commits November 13, 2018 11:44

Made PR Changes

241a4fe

Fixed refresh tests for intersection observer

05f8046

jridgewell reviewed Nov 13, 2018

View reviewed changes

torch2424 added 2 commits November 13, 2018 14:38

Removed returns

e75f2b5

Fixed some linting errors

14fc692

zhouyx reviewed Nov 14, 2018

View reviewed changes

Made PR Changes

095f94f

zhouyx approved these changes Nov 14, 2018

View reviewed changes

torch2424 added 2 commits November 14, 2018 16:32

Merge branch 'master' of https://github.com/torch2424/amphtml into fa…

9bf91c1

…ndango-bug-fix-no-example

Fixed linting errors

5861c3b

jridgewell approved these changes Nov 15, 2018

View reviewed changes

torch2424 merged commit 2a74533 into ampproject:master Nov 16, 2018

torch2424 deleted the fandango-bug-fix-no-example branch November 16, 2018 00:18

torch2424 mentioned this pull request Nov 17, 2018

IntersectionObserver not dispatching event until scroll #19370

Closed

ampprojectbot added the PR use: In Beta / Experimental label Nov 21, 2018

ampprojectbot added the PR use: In Stable label Nov 28, 2018

robmurtagh mentioned this pull request Jan 28, 2019

Major change to amp-ad viewability reporting #19821

Closed

🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168

🐛Intersection Observer Polyfill Mutation Observer - Fixed Race Condition #19168

Conversation

torch2424 commented Nov 7, 2018

Example

Choose a reason for hiding this comment

torch2424 commented Nov 7, 2018

zhouyx commented Nov 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 commented Nov 10, 2018

torch2424 commented Nov 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 commented Nov 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torch2424 commented Nov 14, 2018

zhouyx left a comment

Choose a reason for hiding this comment

torch2424 commented Nov 15, 2018