lit / lit-element

LEGACY REPO. This repository is for maintenance of the legacy LitElement library. The LitElement base class is now part of the Lit library, which is developed in the lit monorepo.
https://lit-element.polymer-project.org
BSD 3-Clause "New" or "Revised" License
4.49k stars 318 forks source link

Sometimes adoptStyles is not called #1158

Closed bschlenk closed 3 years ago

bschlenk commented 3 years ago

I haven't come up with a good repro for this yet, but I'm running into an odd bug.

There seems to be a race condition with this line of code

if (window.ShadowRoot && this.renderRoot instanceof window.ShadowRoot) {

This has only been reproducible for us when telling chrome to disable cache and simulate slow 3g connection.

Intermittently, the this.renderRoot instanceof window.ShadowRoot check is returning false. I can verify this on my page by pasting this check into the chrome console. My components are not in an iframe, so this doesn't seem to be related to https://github.com/Polymer/lit-element/issues/1139.

If I run $0.renderRoot.ownerDocument === document I get true. So I have no idea why sometimes the instanceof check would fail. Do you have any ideas? I'm also wondering if there might be a better check to perform here. Maybe something like ('' + this.renderRoot).includes('ShadowRoot')?

Some other details

justinfagnani commented 3 years ago

I'm really unsure what race condition could be happening here. this.renderRoot is created synchronously on the line before. if you haven't overridden createRenderRoot() is will be a ShadowRoot. I can't see why this.renderRoot.constructor.name would be ShadowRoot, but instanceof would fail. If you're not moving elements from an iframe this sounds like a browser bug.

Which browser/versions do you see this on? And can you detail your methodology for testing? I'd like to reproduce it.

bschlenk commented 3 years ago

Yeah it is really bizarre. I'm seeing this on Chrome 88.0.4324.182 on mac 10.14.6, lit-element 2.4.0.

Haven't been able to reproduce on FF or Safari. So it certainly could be a Chrome bug.

For now we have this workaround in our base class that extends LitElement:

  initialize() {
    super.initialize();
    if (
      this.renderRoot !== this &&
      (this.renderRoot as any).adoptedStyleSheets?.length === 0
    ) {
      this.adoptStyles();
    }
  }

This works for us because we only ever override createRenderRoot to do return this. I'm not sure what this could break in the scenario where the browser needs polyfills.

bschlenk commented 3 years ago

Here's a gist that should repro it with disable cache and slow 3g set. You'll know you've hit the bug when "Hello from MyElement!" is not rendered red. https://gist.github.com/bschlenk/40c989eb0287736d62ab0ad192e5c47c

I know there's a lot of junk on this page, but that's the minimum we've seen that causes the issue. It very well could be something odd going on in one of those other scripts.

sorvell commented 3 years ago

Here is a slightly simpler repro without LitElement: https://output.jsbin.com/conutug/1.

With your scripts setup, this is enough to make it break:

class MyElement extends HTMLElement {
  constructor() {
    super();
    this.attachShadow({mode: 'open'});
    // when using slow 3g and no caching, this displays false.
    console.log('Is this.shadowRoot instanceof window.ShadowRoot?', this.shadowRoot instanceof window.ShadowRoot);
  }
}
bschlenk commented 3 years ago

If it helps, that window.BOOMER thing is a modified version of https://github.com/akamai/boomerang.

This doesn't look like a lit-element bug then. Do you think I should open a bug report with Chrome?

kevinpschaaf commented 3 years ago

I was able to repro your issue using the code in the gist.

What I've found is that somehow the ShadowRoot on my-element is using the ShadowRoot constructor from the iframe in that page, which at least explains the root cause for the issue:

> $('my-element').renderRoot instanceof window.ShadowRoot
< false

> $('my-element').renderRoot instanceof $('iframe').contentWindow.ShadowRoot
< true

How that's happening is a bit of a mystery. We confirmed that none of ShadowRoot, HTMLElement, HTMLElement.prototype, HTMLElement.prototype.attachShadow, etc. appear to be modified. Calling attachShadow on a div works as expected:

> document.createElement('div').attachShadow({mode:'open'}) instanceof ShadowRoot
< true

So this seems like a browser bug. We filed a chrome bug here: https://bugs.chromium.org/p/chromium/issues/detail?id=1182356

kevinpschaaf commented 3 years ago

@bschlenk See https://bugs.chromium.org/p/chromium/issues/detail?id=1182356#c3; the Chrome engineer is asking for a more reduced reproduction. We're not familiar with the boomerang library or how it might affect the JS/DOM environment; can I suggest you either try digging into that or filing a bug with boomerang to try and figure out how to reduce the issue?

sorvell commented 3 years ago

Closing this issue since this is a Chrome bug. See https://bugs.chromium.org/p/chromium/issues/detail?id=1182356