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.

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

Fix ensureLoaded-vs-build race condition in HEAD #32979

Merged
merged 1 commit into from Mar 1, 2021

Conversation

dvoytenko
Copy link
Contributor

@dvoytenko dvoytenko commented Feb 28, 2021

Closes #32978

This is the #32980 for the current HEAD

@dvoytenko dvoytenko changed the title Fix for build race condition on ensureLoaded Fix ensureLoaded-vs-build race condition in main branch Feb 28, 2021
@dvoytenko dvoytenko changed the title Fix ensureLoaded-vs-build race condition in main branch Fix ensureLoaded-vs-build race condition in HEAD Feb 28, 2021
@dvoytenko dvoytenko marked this pull request as ready for review February 28, 2021 11:28
@dvoytenko dvoytenko merged commit abcf9a4 into ampproject:master Mar 1, 2021
@dvoytenko dvoytenko deleted the run3/lightbox-list branch March 1, 2021 17:29
@@ -1584,7 +1584,8 @@ describes.realWin('CustomElement', {amp: true}, (env) => {
.once();

const promise = element.ensureLoaded(parentPriority);
await element.buildInternal();
await resource.build();
await resource.whenBuilt();
Copy link
Member

Choose a reason for hiding this comment

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

is whenBuilt different from resource.build()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Unfortunately it is. This is in a nutshell exactly why the bug happened. This is just a quick fix. Our big-fix options:

  1. Reimplement Resource.whenBuild and state.
  2. Transition some elements to V1 faster. V1 do not have that problem by definition.

@@ -602,26 +602,31 @@ function createBaseCustomElementClass(win) {
return this.whenLoaded();
}

// Very ugly! The "built" signal must be resolved from the Resource
// and not the element itself because the Resource has not correctly
// set its state for the downstream to process it correctly.
Copy link
Member

Choose a reason for hiding this comment

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

what is downstream? would it be cleanest for custom-element to force built signal on Resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, no. The problem is not only a built signal, but also all this code:

this.isBuilding_ = false;
// With IntersectionObserver, measure can happen before build
// so check if we're "ready for layout" (measured and built) here.
if (this.intersect_ && this.hasBeenMeasured()) {
this.state_ = ResourceState.READY_FOR_LAYOUT;
// The InOb premeasurement couldn't account for fixed position since
// implementation wasn't loaded yet. Perform a remeasure now that we know it
// is fixed.
if (this.element.isAlwaysFixed() && !this.isFixed_) {
this.requestMeasure();
}
this.element.onMeasure(/* sizeChanged */ true);
} else {
this.state_ = ResourceState.NOT_LAID_OUT;
}
// TODO(dvoytenko): merge with the standard BUILT signal.
this.element.signals().signal('res-built');

What we could do is to refactor that code to be called by the CustomElement itself in the same microtask as its own build completion code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would actually be quite easy to achieve. But I still wonder if doing this would take away time from maybe porting a few more elements to V1 to address most of the issues. Let me know what you think.

Copy link
Member

@samouri samouri Mar 2, 2021

Choose a reason for hiding this comment

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

Is it that both Resource and CustomElement both have their own isBuilt state and we need to dedupe them or are there other issues? I'm in favor of deduping as much as possible between Resource and CustomElement (sizes, states, etc). It has caused multiple P0s so far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Big area of concern. V1 elements so far simply do not use Resource interface at all (i.e. they are already deduped). We can definitely dedupe V0 elements as well area-by-area.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Latest release Breaks amp-youtube,amp-img on an amp-list inside an lightbox
4 participants