Skip to content

Commit

Permalink
amp-bind: Fix state/ready race condition (#21033)
Browse files Browse the repository at this point in the history 
* First pass on fixing race condition.

* Fix amp-state tests.

* Fix test-bind-impl.js.

* Check for unbuilt <amp-state> instead of N setState() calls.

* Tweak comments.

* Add userAssert for 'src' attribute in refresh action.
  • Loading branch information
William Chou committed Feb 25, 2019
1 parent 0b29454 commit 4746f19
Show file tree
Hide file tree
Showing 4 changed files with 144 additions and 87 deletions.
84 changes: 40 additions & 44 deletions extensions/amp-bind/0.1/amp-state.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,14 +50,34 @@ 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', () => {
userAssert(this.element.hasAttribute('src'),
'Can\'t refresh <amp-state> without "src" attribute.');
this.fetchAndUpdate_(/* isInit */ false, /* opt_refresh */ true);
}, ActionTrust.HIGH);
}

/** @override */
mutatedAttributesCallback(mutations) {
const viewer = Services.viewerForDoc(this.getAmpDoc());
const viewer = Services.viewerForDoc(this.element);
if (!viewer.hasBeenVisible()) {
const TAG = this.getName_();
dev().error(TAG, 'Viewer must be visible before mutation.');
Expand All @@ -75,40 +95,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 +113,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 All @@ -134,7 +128,6 @@ export class AmpState extends AMP.BaseElement {
*/
fetch_(ampdoc, element, isInit, opt_refresh) {
const src = element.getAttribute('src');

// Require opt-in for URL variable replacements on CORS fetches triggered
// by [src] mutation. @see spec/amp-var-substitutions.md
let policy = UrlReplacementPolicy.OPT_IN;
Expand All @@ -154,10 +147,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.element);
return viewer.whenFirstVisible()
.then(() => this.fetch_(ampdoc, this.element, isInit, opt_refresh))
.then(json => this.updateState_(json, isInit));
}

/**
Expand All @@ -170,12 +164,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
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,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 @@ -217,10 +221,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 @@ -230,12 +234,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 @@ -270,7 +275,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 @@ -450,7 +454,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 @@ -471,10 +475,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 @@ -542,7 +569,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 @@ -997,16 +1024,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
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

0 comments on commit 4746f19

Please sign in to comment.