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

amp-list on shadow-dom doesn't render mustache template #21902

Closed
iwoak opened this issue Apr 18, 2019 · 39 comments
Closed

amp-list on shadow-dom doesn't render mustache template #21902

iwoak opened this issue Apr 18, 2019 · 39 comments

Comments

@iwoak
Copy link

iwoak commented Apr 18, 2019

What's the issue?

amp-list on shadow-dom doesn't render mustache template from yesterday (17 April)

image

How do we reproduce the issue?

You can visit every ours detail article page on https://rep.repubblica.it for example https://rep.repubblica.it/pwa/intervista/2019/04/17/news/orlando_il_ministro_sappia_che_non_mi_faro_scavalcare_sono_pronto_a_denunciarlo_-224314656/ and you can look for vote and comments info and links at the right of "condividi" (share) link

  1. Go to our premium home page news site on https://rep.repubblica.it
  2. Click on any link to an article detail page (eg https://rep.repubblica.it for example https://rep.repubblica.it/pwa/intervista/2019/04/17/news/orlando_il_ministro_sappia_che_non_mi_faro_scavalcare_sono_pronto_a_denunciarlo_-224314656/)
  3. Find share bar after the title and signature like this
    image
  4. You can find only share label, miss vote and comments label

If you visit pure amp page at this url https://rep.repubblica.it/ws/detail/intervista/2019/04/17/news/orlando_il_ministro_sappia_che_non_mi_faro_scavalcare_sono_pronto_a_denunciarlo_-224314656/ and all works fine

Ours rest json services linked on src attribute at amp-list works fine and response with data and 200 http code.

What browsers are affected?

Chrome, Firefox, Safari

Which AMP version is affected?

1904091426070

@aghassemi
Copy link
Contributor

Hi @iwoak.

Does this always happen? I have not been able to reproduce in Chrome/Safari/Firefox both comment and rating sections show up fine for me with 1904091426070 ( I am testing by going to https://rep.repubblica.it/pwa/generale/2019/04/17/news/iva_tria_non_esclude_i_rincari_lega_e_5_stelle_mai_con_noi_-224316802/ )

Also note that 1904091426070 went to production April 15

@aghassemi
Copy link
Contributor

/cc @prateekbh

@aghassemi aghassemi self-assigned this Apr 18, 2019
@aghassemi
Copy link
Contributor

@iwoak

The only time I was able to repro this was
1- clear cache
2- hit https://rep.repubblica.it instead of the article directly
3- click on an article.

In that case, I got the following errors from the service-worker:

  | Promise.then (async) |   |  
-- | -- | -- | --
  | (anonymous) | @ | service-worker.js:279
  | Promise.then (async) |   |  
  | (anonymous) | @ | service-worker.js:279
  | Promise.then (async) |   |  
  | i | @ | service-worker.js:279
  | (anonymous) | @ | service-worker.js:279
  | Promise.then (async) |   |  
  | r | @ | service-worker.js:279
  | (anonymous) | @ | service-worker.js:279
  | r | @ | service-worker.js:279

upon next refresh, everything worked and no service-worker errors appeared.

Any chance your service-worker updated around that time frame? I can see amp-list trying to fetch the endpoint but service worker fails to respond leaving it in loading mode.

@iwoak
Copy link
Author

iwoak commented Apr 19, 2019

Hi @aghassemi and thanks for your replies, but I still see the problem and I can replicate it in both way (visiting directly detail page or navigate from home page).

image

I don't receive any error from service-worker :(

@aghassemi
Copy link
Contributor

aghassemi commented Apr 19, 2019

@iwoak Thanks, how often does this repro for you? (like, how many times after refreshing) I still haven't been able to hit it. Using Firefox 66 I get the following everytime i refresh
Screen Shot 2019-04-19 at 7 49 37 AM

There likely is some sort of a race condition here. Only recent change related to amp-list in 1904091426070 is #21512 adding @choumx to take a look as well.

@aghassemi
Copy link
Contributor

aghassemi commented Apr 19, 2019

Chatted with @prateekbh, he will take a look. I doubt that this is new but recent changes may have caused the race to be more common. I still haven't been able to repro more than couple of times.

@prateekbh
Copy link
Member

prateekbh commented Apr 19, 2019

While investigating this, I did observe that the social calls are failing intermittently. Screenshots attached.

This is pretty frequent and happened over a few refreshes
Screen Shot 2019-04-19 at 1 52 09 PM
Screen Shot 2019-04-19 at 1 53 34 PM

@prateekbh
Copy link
Member

@choumx I found this which is similar to #19511. Should this be converted to this.getAmpDoc()

@justforfun
Copy link

I'm able to reproduce the problem on Chrome at every hit of this page:
https://rep.repubblica.it/pwa/intervista/2019/04/19/news/di_maio_sulla_legge_decidono_i_giudici_salvini_non_faccia_come_berlusconi_-224471120/

while on the plain AMP page everything is working as expected:

https://rep.repubblica.it/ws/detail/intervista/2019/04/19/news/di_maio_sulla_legge_decidono_i_giudici_salvini_non_faccia_come_berlusconi_-224471120/

I don't see any service worker related error
Schermata 2019-04-20 alle 23 21 01

@prateekbh
Copy link
Member

Hi @justforfun, while we are trying to figure out the race condition can you please verify that the same happens for you if the service worker is not registered or is bypassed by devtools.

Screen Shot 2019-04-23 at 4 56 00 PM

@iwoak
Copy link
Author

iwoak commented Apr 25, 2019

Hi @prateekbh, both me and @justforfun has the same problem even with SW not registered or bypassed.

@prateekbh
Copy link
Member

Thanks this helps us narrow down the problem 🙂

@sparhami
Copy link

I remember debugging a similar sort of issue with shadow DOM and amp-access's template rendering (#17328). The problem had to do with viewerCanRenderTemplates returning false when called within Shadow DOM.

I did make a change to fix that portion (#17896), but it could still be a problem or something similar (perhaps the wrong amp doc is being used somewhere). Could also try the ampdoc-closest experiment (either disable it if it is enabled, or enable it if it is not).

@iwoak
Copy link
Author

iwoak commented Apr 26, 2019

Hi @sparhami I have this problem every time so I could debug if you write me some instructions.
I confirm: viewerCanRenderTemplates returning false

image

@iwoak
Copy link
Author

iwoak commented Apr 26, 2019

I found an elaboration difference from amp pure page and amp in shadow dom:

image

"ReferenceError: sc is not defined at eval (eval at sc (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:65:64), <anonymous>:1:1) at sc (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:65:64) at Y.push.f.Y.compileCallback (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:64:370) at Y.Gf (https://cdn.ampproject.org/shadow-v0.js:134:249) at new Y (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:63:275) at https://cdn.ampproject.org/shadow-v0.js:138:431"

  • and after in console i can see
    Uncaught (in promise) TypeError: Cannot read property 'ownerDocument' of undefined at je (shadow-v0.js:formatted:2464) at Cm (shadow-v0.js:formatted:11638) at wm.f.observeHiddenMutations (shadow-v0.js:formatted:11626) at wm.f.setup (shadow-v0.js:formatted:11619) at shadow-v0.js:formatted:12984

  • on debug mode I can see correct rendered template by mustache on renderTokens method:
    "<span class="icon-comment"></span><span class="label" [hidden]="6 != 0">Commenta</span><span class="label" [hidden]="6 != 1">6 Commento</span><span class="label" [hidden]="6 <= 1">6 Commenti</span>"

  • and after all the generated html fragment without rendered template
    <amp-list src="https://utilitysocial.repubblica.it/comments/index.php?geturl=CANONICAL_URL&amp;site=rep&amp;rnd=RANDOM" layout="fixed" width="200" height="35" class="i-amphtml-element i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-layout" i-amphtml-layout="fixed" aria-live="polite" style="width: 200px; height: 35px;"><template type="amp-mustache"><span class="icon-comment"></span><span class="label" [hidden]="{{commentCount}} != 0">Commenta</span><span class="label" [hidden]="{{commentCount}} != 1">{{commentCount}} Commento</span><span class="label" [hidden]="{{commentCount}} <= 1">{{commentCount}} Commenti</span></template><div role="list" class="i-amphtml-fill-content i-amphtml-replaced-content"></div><div class="i-amphtml-loading-container i-amphtml-fill-content"><div class="i-amphtml-loader amp-active"> <div class="i-amphtml-loader-dot"></div> <div class="i-amphtml-loader-dot"></div> <div class="i-amphtml-loader-dot"></div> </div></div></amp-list>

Another information: I have different connection lines here, with my developer line I have the problem every time (more or less 300mb), but if i plug my laptop to lan for visitors in a meeting room everything works fine (approximately 50mb)

@sparhami
Copy link

Hi @sparhami I have this problem every time so I could debug if you write me some instructions.
I confirm: viewerCanRenderTemplates returning false

Sorry I think I might have had that backwards. I think this may be fine.

@prateekbh
Copy link
Member

just adding @choumx @dvoytenko if they have any idea

@iwoak
Copy link
Author

iwoak commented Apr 27, 2019

@sparhami, what do you think about this?

I found an elaboration difference from amp pure page and amp in shadow dom:

image

"ReferenceError: sc is not defined at eval (eval at sc (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:65:64), <anonymous>:1:1) at sc (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:65:64) at Y.push.f.Y.compileCallback (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:64:370) at Y.Gf (https://cdn.ampproject.org/shadow-v0.js:134:249) at new Y (https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js:63:275) at https://cdn.ampproject.org/shadow-v0.js:138:431"

@gilbertococchi
Copy link

Hi all, I can confirm that the SW "service-worker.js:1 Uncaught (in promise) DOMException" errors because of Quota Errors are unrelated as I can get the Comments to work with the DOMException error there.

I've noticed that the error occurs 100% when this error is visible from the Console:

blob:https://rep.repubblica.it/8a96ddc0-0b73-4f68-b721-2b3b1b8fc296:6 Uncaught TypeError: self.reportError is not a function
    at qa.r.error (blob:https://rep.repubblica.it/8a96ddc0-0b73-4f68-b721-2b3b1b8fc296:6)
    at blob:https://rep.repubblica.it/8a96ddc0-0b73-4f68-b721-2b3b1b8fc296:78

blob:https://rep.repubblica.it/8a96ddc0-0b73-4f68-b721-2b3b1b8fc296

Screenshot 2019-04-29 at 13 21 40

Steps to reproduce constantly:

  • Open New Anonymous Tab:
  • Browse https://rep.repubblica.it with Mobile Emulation "Pixel 2XL" and Network "Fast 3G".
  • Wait for the Homepage to load entirely.
  • Click on the first article to get the issue on Comments (wait for article to load entirely on Fast 3G emulation).

@prateekbh
Copy link
Member

Will use this today to dig deeper today

@prateekbh
Copy link
Member

prateekbh commented Apr 29, 2019

Confirmed: I can reproduce this in an incognito window using the steps mentioned in: #21902 (comment)

@sparhami
Copy link

The self.reportError part seems to be #20938.

@choumx Can we prioritize fixing this?

@prateekbh
Copy link
Member

The stacktrace i am getting is pointing to https://github.com/ampproject/amphtml/pull/21687/files#diff-a244d3899d7cb3930087a9d566a46af9R259

Which should be fixed with #21976,I'll re-check tomorrow with the dev channel once the opt-in is rolled out

@gilbertococchi
Copy link

Hi Prateek, thank you for checking on this.

Did you manage to test it on Dev Channel with the fix?

I also noticed sometimes randomly the Comments via amp-list loads even when I am getting the "self.reportError is not a function" error...

Screenshot 2019-04-30 at 17 10 30

@prateekbh
Copy link
Member

@gilbertococchi the dev channel fix will be available today, will update the thread once I check it. If there are any more errors they are definitely masked by this thus waiting for this fix.

Also the self.reportError is a known error and I guess @choumx/ @sparhami are working on it

@prateekbh
Copy link
Member

Hi I tested this with canary: https://cdn.ampproject.org/rtv/001904301721170/shadow-v0.js with a custom redirect and seems like it fixes the problem.

The canary will be available as an opt-in tomorrow and you should be able to check with it as well.

If everything works i'll continue adding tests around this.

@prateekbh
Copy link
Member

Hi I just tested with the dev channel and it seems to be working for me with the above steps.

Can you please check and revert

@iwoak
Copy link
Author

iwoak commented May 2, 2019

Hi @prateekbh with the dev channel it works fine even for me. Thanks a lot!
When could be made a release? Just for understand if we can wait the release or we must found an other temp fix.

@prateekbh
Copy link
Member

I guess I can get this cherry-picked for you today :)

@prateekbh
Copy link
Member

Hi @iwoak We did a cherry pick of a suspected change whose fix is available in production now. But I can still the issue.

I'll try to look for any other suspects.

The complete canary will reach production on Tuesday, but will try to ask if this can be done any sooner.

@prateekbh
Copy link
Member

// @cathyxz

@iwoak
Copy link
Author

iwoak commented May 3, 2019

I still see the issue too, but thanks a lot @prateekbh for cherry pick!

@mattwomple
Copy link
Member

mattwomple commented May 3, 2019

@prateekbh (and CC: @iwoak ) Based on these comments:

gilbertococchi

I've noticed that the error occurs 100% when this error is visible from the Console

aghassemi

The only time I was able to repro this was
1- clear cache
2- hit https://rep.repubblica.it instead of the article directly
3- click on an article.

... the issue sounds directly related to #17532 . If this is the case, it can be remedied in one of two ways:

@cathyxz
Copy link
Contributor

cathyxz commented May 3, 2019

So @prateekbh figured out that 1904091426070 was the release that broke it and our new dev channel contains changes that fix it. A breakpoints indicate that the main difference here (to <amp-list>) between throttling to Fast 3G is whether or not the updateBindings_ promise ever returns to result in a call to render_, which is the function that appends the elements to the DOM.

updateBindings_ in both cases, end up waiting for a scanAndApply function in bind-impl.js which never returns in the Fast 3G case. The relevant code is:

scanAndApply(addedElements, removedElements, opt_timeout) {
// TODO(choumx): Dependency on init may be removed if we skip elements
// with .i-amphtml-binding during tree walk and extract macro setup.
return this.initializePromise_.then(() => {
return this.rescan_(addedElements, removedElements, opt_timeout || 2000);
});
}

In the Fast 3G case where the bug repros, a breakpoint on L387 at rescan never hits, whereas it does when we unthrottle the network.

If I may venture a hypothesis, the most closely related PR that might cause this from release 1904091426070 was #21763 (which adds a wait to initializePromise_ in scanAndApply, the most closely related PR that might fix it might be #21854 as @mattwomple says above.

@prateekbh
Copy link
Member

// @choumx as he might have better idea

@dreamofabear
Copy link

Cathy was on the right track. The problem is there are actually two shadow AMP docs on this page: (1) the main article and (2) a subscriptions widget in the top right corner.

Both of these shadow docs use amp-bind, but amp-bind's worker is only scoped to support 1 doc per window. This is the cause of the worker console error -- the second doc's amp-bind initialization always fails (before the fix in canary).

So the race is actually "which page gets initialized first". If the article doc happens to be initialized after the subscriptions, then the initialization promise never resolves. This means amp-list render gets stuck waiting for amp-bind to rescan.

The "fix" in canary just ignores this error and overwrites the existing worker state. This likely breaks amp-bind functionality somewhere, but it does allow the promise to resolve. :)

The long-term fix should be to (A) support multiple docs in amp-bind's worker and (B) make cross-extension APIs more resilient (promise timeouts etc.) and (C) fix error logging in web worker. Happy to spend some cycles on this next week.

@prateekbh
Copy link
Member

Thank you @choumx 🤗
@iwoak the canary rolls out to production on tuesday, but if you need an immediate fix you can load shadow-v0.js from https://cdn.ampproject.org/rtv/011904301721170/shadow-v0.js instead of https://cdn.ampproject.org/shadow-v0.js

@iwoak
Copy link
Author

iwoak commented May 6, 2019

Thanks a lot @prateekbh and @choumx! the problem is very clear now.
We look forward fo tuesday production release and we'll work to remove the less important second amp shadow dom (subscriptions widget in the top right corner)

@iwoak
Copy link
Author

iwoak commented May 20, 2019

thanks a lot, now all works fine online for us on rep.repubblica.it. Until the long-term fix is published maybe you could put a note on the documentation about only one shadow amp tip.

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

No branches or pull requests

10 participants