Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 20 commits into from Nov 16, 2018
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7ef9930
Got a working half-broken page that we can test with
torch2424 Nov 6, 2018
ad491aa
COnfirmed the bug is that the pass isn't long enough
torch2424 Nov 6, 2018
63caf4b
Going to go home. But I learned that it doesn't fire until the child …
torch2424 Nov 6, 2018
2be1f3b
I was doing something
torch2424 Nov 6, 2018
59f561e
Fixed the polyfill completely
torch2424 Nov 6, 2018
c17cb5c
Fixed linting
torch2424 Nov 7, 2018
87ec925
Fixed all precommit checks
torch2424 Nov 7, 2018
f96fd1b
Removed the one-off fandango example
torch2424 Nov 7, 2018
0eba342
Made PR Changes
torch2424 Nov 7, 2018
d5051b9
Created an onNextPass for resources
torch2424 Nov 10, 2018
9892c8e
Fixed tests for ampdoc
torch2424 Nov 10, 2018
42a9964
Removed the original getLayoutRect
torch2424 Nov 10, 2018
c7282f8
Removed a dangling opt_calledFromMutationObserver
torch2424 Nov 10, 2018
241a4fe
Made PR Changes
torch2424 Nov 13, 2018
05f8046
Fixed refresh tests for intersection observer
torch2424 Nov 13, 2018
e75f2b5
Removed returns
torch2424 Nov 13, 2018
14fc692
Fixed some linting errors
torch2424 Nov 13, 2018
095f94f
Made PR Changes
torch2424 Nov 14, 2018
9bf91c1
Merge branch 'master' of https://github.com/torch2424/amphtml into fa…
torch2424 Nov 15, 2018
5861c3b
Fixed linting errors
torch2424 Nov 15, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
65 changes: 38 additions & 27 deletions src/intersection-observer-polyfill.js
View file
Open in desktop
Expand Up @@ -14,7 +14,6 @@
* limitations under the License.
*/

import {Pass} from './pass';
import {Services} from './services';
import {SubscriptionApi} from './iframe-helper';
import {dev} from './log';
Expand Down Expand Up @@ -245,8 +244,8 @@ export class IntersectionObserverPolyfill {
/** @private {?./service/viewport/viewport-impl.Viewport} */
this.viewport_ = null;

/** @private {?Pass} */
this.mutationPass_ = null;
/** @private {?./service/resources-impl.Resources} */
this.resources_ = null;
}

/**
Expand Down Expand Up @@ -295,11 +294,7 @@ export class IntersectionObserverPolyfill {
const ampdoc = Services.ampdoc(element);
if (ampdoc.win.MutationObserver && !this.mutationObserver_) {
this.viewport_ = Services.viewportForDoc(element);
this.mutationPass_ = new Pass(ampdoc.win, () => {
if (this.viewport_) {
this.tick(this.viewport_.getRect());
}
});
this.resources_ = Services.resourcesForDoc(ampdoc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 😮

this.mutationObserver_ = new ampdoc.win.MutationObserver(
this.handleMutationObserverNotification_.bind(this)
);
Expand Down Expand Up @@ -339,8 +334,9 @@ export class IntersectionObserverPolyfill {
* The iframe must be a non-scrollable iframe.
* @param {!./layout-rect.LayoutRectDef} hostViewport
* @param {./layout-rect.LayoutRectDef=} opt_iframe
* @param {boolean=} opt_calledFromMutationObserver
*/
tick(hostViewport, opt_iframe) {
tick(hostViewport, opt_iframe, opt_calledFromMutationObserver) {
if (opt_iframe) {
// If element inside an iframe. Adjust origin to the iframe.left/top.
hostViewport =
Expand All @@ -356,7 +352,11 @@ export class IntersectionObserverPolyfill {

for (let i = 0; i < this.observeEntries_.length; i++) {
const change = this.getValidIntersectionChangeEntry_(
this.observeEntries_[i], hostViewport, opt_iframe);
this.observeEntries_[i],
hostViewport,
opt_iframe,
opt_calledFromMutationObserver
);
if (change) {
changes.push(change);
}
Expand All @@ -376,22 +376,37 @@ export class IntersectionObserverPolyfill {
* @param {!ElementIntersectionStateDef} state
* @param {!./layout-rect.LayoutRectDef} hostViewport hostViewport's rect
* @param {./layout-rect.LayoutRectDef=} opt_iframe iframe container rect
* If opt_iframe is provided, all LayoutRect has position relative to
* the iframe. If opt_iframe is not provided,
* all LayoutRect has position relative to the host document.
* @param {boolean=} opt_calledFromMutationObserver
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
  }
}

* @return {?IntersectionObserverEntry} A valid change entry or null if ratio
* @private
*/
getValidIntersectionChangeEntry_(state, hostViewport, opt_iframe) {
getValidIntersectionChangeEntry_(state, hostViewport,
opt_iframe, opt_calledFromMutationObserver) {
const {element} = state;

// Normalize container LayoutRect to be relative to page
let elementRect = null;
const owner = element.getOwner();
let ownerRect = null;

// If opt_iframe is provided, all LayoutRect has position relative to
// the iframe.
// If opt_iframe is not provided, all LayoutRect has position relative to
// the host document.
const elementRect = element.getLayoutBox();
const owner = element.getOwner();
ownerRect = owner && owner.getLayoutBox();
// If opt_calledFromMutationObserver is provided,
// we need to get our bounding rect rather
// Than our layout box, in order to avoid race conditions
// with the resource scheduler
if (opt_calledFromMutationObserver) {
elementRect = this.viewport_.getLayoutRect(element);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ownerRect = owner && this.viewport_.getLayoutRect(owner);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These need to be inside measure phases.

} else {
elementRect = element.getLayoutBox();
ownerRect = owner && owner.getLayoutBox();
}

// calculate intersectionRect. that the element intersects with hostViewport
// and intersects with owner element and container iframe if exists.
Expand Down Expand Up @@ -420,12 +435,11 @@ export class IntersectionObserverPolyfill {
* @private
*/
handleMutationObserverNotification_() {
if (this.mutationPass_.isPending()) {
return;
if (this.viewport_ && this.resources_) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to check this.

this.resources_.onNextPass(() => {
this.tick(this.viewport_.getRect(), undefined, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
}

// Wait one animation frame so that other mutations may arrive.
this.mutationPass_.schedule(16);
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Useless return.

}

Expand All @@ -439,10 +453,7 @@ export class IntersectionObserverPolyfill {
}
this.mutationObserver_ = null;
this.viewport_ = null;
if (this.mutationPass_) {
this.mutationPass_.cancel();
}
this.mutationPass_ = null;
this.resources_ = null;
}
}

Expand Down
20 changes: 18 additions & 2 deletions src/service/resources-impl.js
View file
Open in desktop
Expand Up @@ -211,6 +211,9 @@ export class Resources {
/** @private {boolean} */
this.maybeChangeHeight_ = false;

/** @const @private {!Array<function()>} */
this.passCallbacks_ = [];

/** @private @const {!FiniteStateMachine<!VisibilityState>} */
this.visibilityStateMachine_ = new FiniteStateMachine(
this.viewer_.getVisibilityState()
Expand Down Expand Up @@ -1140,6 +1143,14 @@ export class Resources {
this.schedulePass();
}

/**
* Registers a callback to be called when the next pass happens.
* @param {function()} callback
*/
onNextPass(callback) {
this.passCallbacks_.push(callback);
}

/**
* Runs a pass immediately.
*/
Expand Down Expand Up @@ -1169,7 +1180,7 @@ export class Resources {
this.contentHeight_ = this.viewport_.getContentHeight();
this.viewer_.sendMessage('documentHeight',
dict({'height': this.contentHeight_}), /* cancelUnsent */true);
dev().fine(TAG_, 'document height on load: ' + this.contentHeight_);
dev().fine(TAG_, 'document height on load: %s', this.contentHeight_);
}

const viewportSize = this.viewport_.getSize();
Expand Down Expand Up @@ -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_);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

this.viewport_.contentHeightChanged();
}
});
}

this.passCallbacks_.forEach(callback => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

callback();
});
this.passCallbacks_.length = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down
22 changes: 13 additions & 9 deletions test/functional/test-intersection-observer-polyfill.js
View file
Open in desktop
Expand Up @@ -25,6 +25,12 @@ import {
import {Services} from '../../src/services';
import {layoutRectLtwh} from '../../src/layout-rect';

const fakeAmpDoc = {
getRootNode: () => {return window.document;},
win: window,
isSingleDoc: () => {return true;},
};

describe('IntersectionObserverApi', () => {
let sandbox;
let onScrollSpy;
Expand Down Expand Up @@ -70,12 +76,7 @@ describe('IntersectionObserverApi', () => {
sandbox.stub(Services, 'viewportForDoc').callsFake(() => {
return mockViewport;
});
sandbox.stub(Services, 'ampdoc').callsFake(() => {
return {
getRootNode: () => {return window.document;},
win: window,
};
});
sandbox.stub(Services, 'ampdoc').callsFake(() => fakeAmpDoc);
testEle = {
isBuilt: () => {return true;},
getOwner: () => {return null;},
Expand Down Expand Up @@ -207,6 +208,7 @@ describe('IntersectionObserverPolyfill', () => {
beforeEach(() => {
sandbox = sinon.sandbox;
sandbox.stub(performance, 'now').callsFake(() => 100);
sandbox.stub(Services, 'ampdoc').callsFake(() => fakeAmpDoc);
});

afterEach(() => {
Expand Down Expand Up @@ -304,13 +306,15 @@ describe('IntersectionObserverPolyfill', () => {
},
};
});
sandbox.stub(Services, 'ampdoc').callsFake(() => {
sandbox.stub(Services, 'resourcesForDoc').callsFake(() => {
return {
getRootNode: () => {return window.document;},
win: window,
onNextPass: callback => {
callback();
},
};
});


element = {
isBuilt: () => {return true;},
getOwner: () => {return null;},
Expand Down
28 changes: 28 additions & 0 deletions test/functional/test-resources.js
View file
Open in desktop
Expand Up @@ -1634,6 +1634,34 @@ describe('Resources discoverWork', () => {
});
});
});

describe('passCallbacks_', () => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"onNextPass"


it('should run passCallbacks_ added from onNextPass()', () => {
resources.isRuntimeOn_ = true;
resources.documentReady_ = true;
resources.firstPassAfterDocumentReady_ = true;

const passCallback = sandbox.spy();
resources.onNextPass(passCallback);
resources.doPass();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;


expect(passCallback).to.be.called;
});

it('should remove passCallbacks_ added from onNextPass(), ' +
' after the pass', () => {
resources.isRuntimeOn_ = true;
resources.documentReady_ = true;
resources.firstPassAfterDocumentReady_ = true;

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

expect(resources.passCallbacks_.length).to.be.equal(0);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});
});
});

describes.realWin('Resources contentHeight', {
Expand Down