PolymerElements / iron-overlay-behavior

Makes an element an overlay with an optional backdrop
41 stars 71 forks source link

TypeError: observerHandle is null in Firefox #269

Open TrebuhD opened 6 years ago

TrebuhD commented 6 years ago

Description

On firefox (v58.02), I get the following error after I destroy one particular element which uses iron-overlay-behavior (when navigating to a different route):

TypeError: observerHandle is null

The element in question contains a list of elements, each of them using paper-dropdown-menu. When I delete the paper-dropdown-menu, everything works well. If I include paper-dropdown-menu, even one without any content, it throws this error.

The error is particularly bad, because after it occurs, half of the website refuses to work.

The error seems to be coming from this line:

Polymer.dom(this).unobserveNodes(this._observer);

I'm using webcomponents-lite.js.

Expected outcome

No error is thrown

Actual outcome

Error thrown: TypeError: observerHandle is null

Live Demo & Steps to reproduce

Go to this page, click the downwards facing arrow on the bottom right and then click the green "Pali" button on the left of the element that opened up. The error will occur.

Here's the code for the item containing the dropdown (sc-language-menu on line 200).

Browsers Affected

Stack trace from the error:

unobserveNodes@http://localhost/bower_components/polymer/lib/legacy/polymer.dom.html.js:59:7
detached@http://localhost/bower_components/iron-overlay-behavior/iron-overlay-behavior.html.js:196:7
detached@http://localhost/bower_components/polymer/lib/legacy/class.html.js:242:11
detached@http://localhost/bower_components/polymer/lib/legacy/class.html.js:240:9
detached@http://localhost/bower_components/polymer/lib/legacy/class.html.js:240:9
detached@http://localhost/bower_components/polymer/lib/legacy/class.html.js:240:9
disconnectedCallback@http://localhost/bower_components/polymer/lib/legacy/legacy-element-mixin.html.js:106:9
Fd.prototype.disconnectedCallback@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:136:317
O@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:132:114
we/<@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:147:366
__detachInstance@http://localhost/bower_components/polymer/lib/elements/dom-repeat.html.js:576:9
disconnectedCallback@http://localhost/bower_components/polymer/lib/elements/dom-repeat.html.js:300:9
Fd.prototype.disconnectedCallback@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:136:317
O@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:132:114
we/<@http://localhost/bower_components/webcomponentsjs/webcomponents-lite.js:148:95
__teardownInstance@http://localhost/bower_components/polymer/lib/elements/dom-if.html.js:243:13
__render@http://localhost/bower_components/polymer/lib/elements/dom-if.html.js:147:9
__debounceRender/this.__renderDebouncer<@http://localhost/bower_components/polymer/lib/elements/dom-if.html.js:102:114
setConfig/this._timer<@http://localhost/bower_components/polymer/lib/utils/debounce.html.js:30:9
microtaskFlush@http://localhost/bower_components/polymer/lib/utils/async.html.js:21:11
valdrinkoshi commented 6 years ago

Hi @TrebuhD, I couldn't reproduce the issue through the website, it seems the content is not being served right now. Would you mind providing a smaller repro through a jsbin? e.g. http://jsbin.com/fowahen/4/edit?html,output

Regarding the bug, if this._observer is null it means attached was never invoked, which means there is a bug in polymer itself, e.g. https://github.com/Polymer/polymer/issues/4550 or https://github.com/Polymer/polymer/issues/4049 seem to be related.

TrebuhD commented 6 years ago

Hi @valdrinkoshi . I don't know if I'll be able to produce a jsbin repro, as I don't know what exactly causes this error. I use the exact same elements in other places in the code and it works fine there.

The site is online now though! We had a small bug on production. You can check out the error over there as described in my original post.

I suspect it's a polyfill issue and not a Polymer issue itself - it doesn't happen in Chrome or any browser that has native Shadow DOM/Custom Elements implemented. In any case, I only got this error with iron-overlay-behavior inside paper-dropdown-menu and just in this one instance and my PR fixes it.

TrebuhD commented 6 years ago

Just to let you know, I'll soon be swapping the deployed site to a working version without the error (where I manually edited the line in my local iron-overlay-behavior), so you won't be able to reproduce the error over there anymore. Hopefully, if we could merge that PR, I can avoid creating a fork of iron-overlay-behavior and continue using this repository.

valdrinkoshi commented 6 years ago

The website has minified code, and it's hard to add debugging points where I'd want. Can you provide a smaller repro? Skimming through the codebase, I see that there are several nested dom-repeats and dom-ifs, e.g there are 2 nested ifs, here and here. We cannot merge code without knowing the root cause. Also, each fix requires a unit test to avoid future regressions, so having a smaller repro is needed before we can proceed further.

TrebuhD commented 6 years ago

Thanks for your answer. I'll try to provide a repro soon.

BTW, are nested dom-ifs a bad thing? I had no idea.

valdrinkoshi commented 6 years ago

No it's not bad, it's just hard to debug from my side :) I tried following the data flow but got easily lost, and couldn't easily add breakpoints in the places I wanted to in the minified code :/

snovikov-gd commented 6 years ago

FYI guys, I've faced with the same issue in Edge 16. I've fixed it the same way @TrebuhD is suggesting and have to use a fork of this repo. Unfortunately, I cannot provide repro. I can just say this bug is floating in Edge, sometimes it does not appear in the same conditions. I didn't manage to understand when exactly, but F12 tools opened on the Network tab magically increases possibility it won't appear.

Here you can see there is no '_observer': observer_is_null

And here it fails: observer_is_null_2

It would be great to have this fix merged.

valdrinkoshi commented 6 years ago

This looks very much related to a bug in ShadyDOM where listeners weren't actually removed https://github.com/webcomponents/shadydom/issues/190. This was fixed in shadydom v1.0.13, included in webcomponentsjs v1.1.1 - can you confirm you're using either shadydom v1.0.13 or webcomponents v1.1.1?

synopticum commented 6 years ago

@valdrinkoshi I'm using v1.1.1. I'm sorry, I found a piece of code that manually changes this._observer in runtime so the above is likely related to our codebase, not iron-overlay-behavior component itself.

It would be convenient to have if (this._observer) check though.

fgirardey commented 6 years ago

I'm actually facing the same issue, you can reproduce this bug by forcing polyfills on Chrome.

<script>
    if (window.customElements) window.customElements.forcePolyfill = true;
    ShadyDOM = {force: true};
</script>
<script type="text/javascript" src="webcomponentsjs/webcomponents-loader.js"></script>

I'm actually forced to override the IronOverlayBehaviorImpl.detached method with the same fix than @TrebuhD has commited.

Why waiting so long to release the fix?

rpapeters commented 6 years ago

I'm also having this issue in (at least) FireFox and created a patch on the Polymer.DomApi.prototype.unobserveNodes function like this:

Polymer.DomApi.prototype.unobserveNodes = function(observerHandle) { observerHandle && observerHandle.disconnect(); }

So the difference is just checking if the observerHandle has a value before calling the disconnect function on it. For me this fixes the issue, what could go wrong here?

aletorrado commented 6 years ago

+1 !!!

aletorrado commented 5 years ago

Any update on this?