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

iwoak · 2019-04-18T13:49:04Z

What's the issue?

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

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

Go to our premium home page news site on https://rep.repubblica.it
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/)
Find share bar after the title and signature like this
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 · 2019-04-18T17:04:33Z

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 · 2019-04-18T17:04:59Z

/cc @prateekbh

aghassemi · 2019-04-18T17:10:43Z

@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 · 2019-04-19T10:37:56Z

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

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

aghassemi · 2019-04-19T14:52:42Z

@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

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 · 2019-04-19T16:08:08Z

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 · 2019-04-19T20:55:01Z

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

prateekbh · 2019-04-19T20:56:35Z

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

justforfun · 2019-04-20T21:21:39Z

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

prateekbh · 2019-04-23T23:56:37Z

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.

iwoak · 2019-04-25T11:21:47Z

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

prateekbh · 2019-04-25T15:31:16Z

Thanks this helps us narrow down the problem 🙂

sparhami · 2019-04-25T23:51:57Z

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 · 2019-04-26T06:18:19Z

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

iwoak · 2019-04-26T06:45:57Z

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

when callstack through in compileCallback here https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js

"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&site=rep&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 · 2019-04-26T17:41:50Z

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 · 2019-04-26T19:22:40Z

just adding @choumx @dvoytenko if they have any idea

iwoak · 2019-04-27T08:32:09Z

@sparhami, what do you think about this?

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

when callstack through in compileCallback here https://cdn.ampproject.org/rtv/011904200955460/v0/amp-mustache-0.2.js

"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 · 2019-04-29T11:25:04Z

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

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 · 2019-04-29T13:46:55Z

Will use this today to dig deeper today

prateekbh · 2019-04-29T17:08:05Z

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

sparhami · 2019-04-29T21:26:30Z

The self.reportError part seems to be #20938.

@choumx Can we prioritize fixing this?

prateekbh · 2019-04-29T22:46:30Z

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 · 2019-04-30T15:10:42Z

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

prateekbh · 2019-04-30T15:57:44Z

@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 · 2019-05-01T01:43:48Z

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 · 2019-05-01T16:44:17Z

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 · 2019-05-02T07:10:01Z

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 · 2019-05-02T15:45:57Z

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

prateekbh · 2019-05-02T23:35:09Z

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 · 2019-05-03T00:11:19Z

// @cathyxz

iwoak · 2019-05-03T06:48:39Z

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

mattwomple · 2019-05-03T19:51:19Z

@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:

AMP project pushes 🐛 web-worker: Do not fail on bind re-init #21854 to production
rep.repubblica.it adds a "cleanup" step at the same time they call shadowDoc.close():
```
delete window.services['amp-worker'];
```

cathyxz · 2019-05-03T20:33:17Z

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:

amphtml/extensions/amp-bind/0.1/bind-impl.js

Lines 383 to 389 in 84a002d

    
           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 · 2019-05-03T20:37:40Z

// @choumx as he might have better idea

dreamofabear · 2019-05-03T21:59:30Z

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 · 2019-05-03T22:03:23Z

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 · 2019-05-06T08:13:42Z

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 · 2019-05-20T09:45:32Z

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.

iwoak added the Type: Bug label Apr 18, 2019

jpettitt added Category: Dynamic/Personalized Content P1: High Priority labels Apr 18, 2019

jpettitt assigned cathyxz and aghassemi Apr 18, 2019

jpettitt added this to the Pending Triage milestone Apr 18, 2019

aghassemi unassigned cathyxz and aghassemi Apr 18, 2019

aghassemi self-assigned this Apr 18, 2019

aghassemi assigned prateekbh and unassigned aghassemi Apr 19, 2019

aghassemi added the Category: PWA label Apr 19, 2019

prateekbh mentioned this issue May 2, 2019

🌸 Cherry-pick request for #21902 into #1904262212040 (Pending) #22113

Closed

This was referenced May 6, 2019

Mid Sprint 2019-05-06 ampproject/wg-performance#2

Closed

Status Update 2019-05-13 ampproject/wg-performance#3

Closed

iwoak closed this as completed May 20, 2019

prateekbh mentioned this issue Jun 10, 2019

Issue using amp-analytics with Shadow DOM #20400

Closed

dreamofabear added the WG: runtime label Jul 8, 2019

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

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

Comments

iwoak commented Apr 18, 2019

What's the issue?

How do we reproduce the issue?

What browsers are affected?

Which AMP version is affected?

aghassemi commented Apr 18, 2019

aghassemi commented Apr 18, 2019

aghassemi commented Apr 18, 2019

iwoak commented Apr 19, 2019

aghassemi commented Apr 19, 2019 • edited Loading

aghassemi commented Apr 19, 2019 • edited Loading

prateekbh commented Apr 19, 2019 • edited Loading

prateekbh commented Apr 19, 2019

justforfun commented Apr 20, 2019

prateekbh commented Apr 23, 2019

iwoak commented Apr 25, 2019

prateekbh commented Apr 25, 2019

sparhami commented Apr 25, 2019

iwoak commented Apr 26, 2019 • edited Loading

iwoak commented Apr 26, 2019 • edited Loading

sparhami commented Apr 26, 2019

prateekbh commented Apr 26, 2019

iwoak commented Apr 27, 2019

gilbertococchi commented Apr 29, 2019

prateekbh commented Apr 29, 2019

prateekbh commented Apr 29, 2019 • edited Loading

sparhami commented Apr 29, 2019

prateekbh commented Apr 29, 2019

gilbertococchi commented Apr 30, 2019

prateekbh commented Apr 30, 2019

prateekbh commented May 1, 2019

prateekbh commented May 1, 2019

iwoak commented May 2, 2019

prateekbh commented May 2, 2019

prateekbh commented May 2, 2019

prateekbh commented May 3, 2019

iwoak commented May 3, 2019

mattwomple commented May 3, 2019 • edited Loading

cathyxz commented May 3, 2019 • edited Loading

prateekbh commented May 3, 2019

dreamofabear commented May 3, 2019

prateekbh commented May 3, 2019

iwoak commented May 6, 2019

iwoak commented May 20, 2019

aghassemi commented Apr 19, 2019 •

edited

Loading

aghassemi commented Apr 19, 2019 •

edited

Loading

prateekbh commented Apr 19, 2019 •

edited

Loading

iwoak commented Apr 26, 2019 •

edited

Loading

iwoak commented Apr 26, 2019 •

edited

Loading

prateekbh commented Apr 29, 2019 •

edited

Loading

mattwomple commented May 3, 2019 •

edited

Loading

cathyxz commented May 3, 2019 •

edited

Loading