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

amp-bind: Fix state/ready race condition #21033

Merged
merged 6 commits into from
Feb 25, 2019
Merged

amp-bind: Fix state/ready race condition #21033

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8e13ca1
First pass on fixing race condition.
Feb 22, 2019
d3d4c61
Fix amp-state tests.
Feb 23, 2019
6a7d157
Fix test-bind-impl.js.
Feb 23, 2019
7ad14fe
Check for unbuilt <amp-state> instead of N setState() calls.
Feb 25, 2019
b1df7ac
Tweak comments.
Feb 25, 2019
dd94adc
Add userAssert for 'src' attribute in refresh action.
Feb 25, 2019
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
79 changes: 37 additions & 42 deletions extensions/amp-bind/0.1/amp-state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,9 +50,27 @@ export class AmpState extends AMP.BaseElement {
toggle(this.element, /* opt_display */ false);
this.element.setAttribute('aria-hidden', 'true');

// Don't parse or fetch in prerender mode.
const viewer = Services.viewerForDoc(this.getAmpDoc());
viewer.whenFirstVisible().then(() => this.initialize_());
const {element} = this;
if (element.hasAttribute('overridable')) {
Services.bindForDocOrNull(element).then(bind => {
devAssert(bind);
bind.addOverridableKey(element.getAttribute('id'));
});
}
// Parse child <script> tag and/or fetch JSON from `src` attribute.
// The latter is allowed to overwrite the former.
const {children} = element;
if (children.length > 0) {
// Bind relies on this happening synchronously in buildCallback().
this.parseAndUpdate_();
}
if (this.element.hasAttribute('src')) {
this.fetchAndUpdate_(/* isInit */ true);
}

this.registerAction('refresh', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I should have made this change when I added refresh support for amp-state but we should only register the action if the element has a src attribute. We should move this block in the if statement above.

if (this.element.hasAttribute('src')) {
  this.fetchAndUpdate_(/* isInit */ true);
  this.registerAction('refresh', ...
}

Copy link
Author

Choose a reason for hiding this comment

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

Good call. Though it's possible for the element to not have a src attribute at buildCallback but have one later (via amp-bind), so I added a userAssert here instead.

this.fetchAndUpdate_(/* isInit */ false, /* opt_refresh */ true);
}, ActionTrust.HIGH);
}

/** @override */
Expand All @@ -75,40 +93,15 @@ export class AmpState extends AMP.BaseElement {
return true;
}

/** @private */
initialize_() {
const {element} = this;
if (element.hasAttribute('overridable')) {
Services.bindForDocOrNull(element).then(bind => {
devAssert(bind, 'Bind service can not be found.');
bind.makeStateKeyOverridable(element.getAttribute('id'));
});
}
// Parse child script tag and/or fetch JSON from endpoint at `src`
// attribute, with the latter taking priority.
const {children} = element;
if (children.length > 0) {
this.parseChildAndUpdateState_();
}
if (this.element.hasAttribute('src')) {
this.fetchAndUpdate_(/* isInit */ true);
}
this.registerAction('refresh', () => {
this.fetchAndUpdate_(/* isInit */ false, /* opt_refresh */ true);
}, ActionTrust.HIGH);
}


/**
* Parses JSON in child script element and updates state.
* Parses JSON in child <script> and updates state.
* @private
*/
parseChildAndUpdateState_() {
parseAndUpdate_() {
const TAG = this.getName_();
const {children} = this.element;
if (children.length != 1) {
this.user().error(
TAG, 'Should contain exactly one <script> child.');
this.user().error(TAG, 'Should contain exactly one <script> child.');
return;
}
const firstChild = children[0];
Expand All @@ -118,8 +111,7 @@ export class AmpState extends AMP.BaseElement {
return;
}
const json = tryParseJson(firstChild.textContent, e => {
this.user().error(
TAG, 'Failed to parse state. Is it valid JSON?', e);
this.user().error(TAG, 'Failed to parse state. Is it valid JSON?', e);
});
this.updateState_(json, /* isInit */ true);
}
Expand Down Expand Up @@ -154,10 +146,11 @@ export class AmpState extends AMP.BaseElement {
*/
fetchAndUpdate_(isInit, opt_refresh) {
const ampdoc = this.getAmpDoc();
return this.fetch_(ampdoc, this.element, isInit, opt_refresh)
.then(json => {
this.updateState_(json, isInit);
});
// Don't fetch in prerender mode.
const viewer = Services.viewerForDoc(this.getAmpDoc());
Copy link
Contributor

Choose a reason for hiding this comment

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

fetchAndUpdate_ is also called in the registered refresh action method call. We should set the viewer service as a private property on the buildCallback so that we don't need to get the service everything time this method is called.

Copy link
Author

Choose a reason for hiding this comment

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

I'd prefer not to do this optimization due to the readability and testing cost. I don't think it makes a perf difference in practice.

return viewer.whenFirstVisible()
.then(() => this.fetch_(ampdoc, this.element, isInit, opt_refresh))
.then(json => this.updateState_(json, isInit));
}

/**
Expand All @@ -170,12 +163,14 @@ export class AmpState extends AMP.BaseElement {
return;
}
const id = userAssert(this.element.id, '<amp-state> must have an id.');
const state = /** @type {!JsonObject} */ (map());
state[id] = json;
Services.bindForDocOrNull(this.element).then(bind => {
devAssert(bind, 'Bind service can not be found.');
bind.setState(state,
/* opt_skipEval */ isInit, /* opt_isAmpStateMutation */ !isInit);
devAssert(bind);
const state = /** @type {!JsonObject} */ (map());
state[id] = json;
// As a rule, initialization should skip evaluation.
// If we're not initializing then this must be a mutation, so we must
// skip <amp-state> evaluation to prevent update cycles.
bind.setState(state, /* skipEval */ isInit, /* skipAmpState */ !isInit);
});
}

Expand Down
58 changes: 42 additions & 16 deletions extensions/amp-bind/0.1/bind-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,11 +183,15 @@ export class Bind {
return this.initialize_(root);
});

/** @private {?Node} */
this.rootNode_ = null;

/** @private {Promise} */
this.setStatePromise_ = null;

/** @private @const {!../../../src/utils/signals.Signals} */
this.signals_ = new Signals();
this.signals_.whenSignal('READY').then(() => this.onReady_());

// Install debug tools.
const g = self.AMP;
Expand All @@ -214,10 +218,10 @@ export class Bind {
* evaluation unless `opt_skipEval` is false.
* @param {!JsonObject} state
* @param {boolean=} opt_skipEval
* @param {boolean=} opt_isAmpStateMutation
* @param {boolean=} opt_skipAmpState
* @return {!Promise}
*/
setState(state, opt_skipEval, opt_isAmpStateMutation) {
setState(state, opt_skipEval, opt_skipAmpState) {
try {
deepMerge(this.state_, state, MAX_MERGE_DEPTH);
} catch (e) {
Expand All @@ -227,12 +231,13 @@ export class Bind {
dev().info(TAG, 'state:', this.state_);

if (opt_skipEval) {
this.checkReadiness_();
return Promise.resolve();
}

const promise = this.initializePromise_
.then(() => this.evaluate_())
.then(results => this.apply_(results, opt_isAmpStateMutation));
.then(results => this.apply_(results, opt_skipAmpState));

if (getMode().test) {
promise.then(() => {
Expand Down Expand Up @@ -267,7 +272,6 @@ export class Bind {
this.maxNumberOfBindings_ = Math.min(2000,
Math.max(1000, this.maxNumberOfBindings_ + 500));

// Signal first mutation (subsequent signals are harmless).
this.signals_.signal('FIRST_MUTATE');

const scope = dict();
Expand Down Expand Up @@ -447,7 +451,7 @@ export class Bind {
* @private
*/
initialize_(root) {
dev().info(TAG, 'init');
this.rootNode_ = root;

// Disallow URL property bindings in AMP4EMAIL.
const allowUrlProperties = !this.isAmp4Email_();
Expand All @@ -468,10 +472,33 @@ export class Bind {
if (getMode().development) {
return this.evaluate_().then(results => this.verify_(results));
}
}).then(() => {
this.viewer_.sendMessage('bindReady', undefined);
this.dispatchEventForTesting_(BindEvents.INITIALIZE);
});
}).then(() => this.checkReadiness_());
}

/**
* Bind is "ready" when its initialization completes _and_ all <amp-state>
* elements' local data is parsed and processed (not remote data).
* @private
*/
checkReadiness_() {
if (!this.rootNode_) {
return;
}
const unbuilt =
this.rootNode_.querySelectorAll('AMP-STATE.i-amphtml-notbuilt');
if (unbuilt.length > 0) {
return;
}
// Use a signal to ensure that onReady_() is only invoked once.
this.initializePromise_.then(() => this.signals_.signal('READY'));
}

/**
* @private
*/
onReady_() {
this.viewer_.sendMessage('bindReady', undefined);
this.dispatchEventForTesting_(BindEvents.INITIALIZE);
}

/**
Expand Down Expand Up @@ -539,7 +566,7 @@ export class Bind {
* a premutate message from the viewer.
* @param {string} key
*/
makeStateKeyOverridable(key) {
addOverridableKey(key) {
this.overridableKeys_.push(key);
}

Expand Down Expand Up @@ -994,16 +1021,15 @@ export class Bind {
/**
* Applies expression results to all elements in the document.
* @param {Object<string, BindExpressionResultDef>} results
* @param {boolean=} opt_isAmpStateMutation
* @param {boolean=} opt_skipAmpState
* @return {!Promise}
* @private
*/
apply_(results, opt_isAmpStateMutation) {
apply_(results, opt_skipAmpState) {
const promises = this.boundElements_.map(boundElement => {
// If this "apply" round is triggered by an <amp-state> mutation,
// ignore updates to <amp-state> element to prevent update cycles.
if (opt_isAmpStateMutation
&& boundElement.element.tagName == 'AMP-STATE') {
// If this evaluation is triggered by an <amp-state> mutation, we must
// ignore updates to any <amp-state> element to prevent update cycles.
if (opt_skipAmpState && boundElement.element.tagName === 'AMP-STATE') {
return Promise.resolve();
}
return this.applyBoundElement_(results, boundElement);
Expand Down
40 changes: 36 additions & 4 deletions extensions/amp-bind/0.1/test/integration/test-bind-impl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,11 +293,42 @@ describe.configure().ifChrome().run('Bind', function() {
});

it('should send "bindReady" to viewer on init', () => {
const signals = bind.signals();
sandbox.spy(signals, 'signal');

expect(signals.signal).to.not.be.called;
expect(viewer.sendMessage).to.not.be.called;

return onBindReady(env, bind).then(() => {
expect(signals.signal).to.be.calledWith('READY');
return signals.whenSignal('READY');
}).then(() => {
expect(viewer.sendMessage).to.be.calledOnce;
expect(viewer.sendMessage).to.be.calledWith('bindReady');
});
});

it('should not send "bindReady" until all <amp-state> are built', () => {
const element = createElement(env, container, '', 'amp-state', true);
element.classList.add('i-amphtml-notbuilt');

const signals = bind.signals();
sandbox.spy(signals, 'signal');

expect(signals.signal).to.not.be.called;
expect(viewer.sendMessage).to.not.be.called;

return onBindReady(env, bind).then(() => {
expect(signals.signal).to.not.be.called;

// Simulate <amp-state> buildCallback().
element.classList.remove('i-amphtml-notbuilt');
bind.setState({}, /* opt_skipEval */ true);

return signals.whenSignal('READY');
}).then(() => {
expect(viewer.sendMessage).to.be.calledOnce;
expect(viewer.sendMessage)
.to.be.calledWithExactly('bindReady', undefined);
expect(viewer.sendMessage).to.be.calledWith('bindReady');
});
});

Expand Down Expand Up @@ -872,8 +903,9 @@ describe.configure().ifChrome().run('Bind', function() {
});

it('should update premutate keys that are overridable', () => {
bind.makeStateKeyOverridable('foo');
bind.makeStateKeyOverridable('bar');
bind.addOverridableKey('foo');
bind.addOverridableKey('bar');

const foo = createElement(env, container, '[text]="foo"');
const bar = createElement(env, container, '[text]="bar"');
const baz = createElement(env, container, '[text]="baz"');
Expand Down