Flow: RangeError with 50 PolymerTemplates and Buttons

Created on 30 May 2019  路  11Comments  路  Source: vaadin/flow

I have a page that displays 50 PolymerTemplates and each template has a single button. Using long polling push, a client side JavaScript error is raised in Chrome when the content is rebuilt. It doesn't look like the error occurs in FireFox, only Chrome.

This may be a client component specific issue, but the issue only seems to occur if I link the Button to the Java code via the @Id attribute so it may be more flow related than a specific client side component.

This issue causes a scalability problem for us because we display a dashboard of recent events, each with a details and acknowledge button. In rare cases, that recent event list can grow to more than 50 items. We can work around it by supporting paging or displaying a subset of the items, but 50 templates/buttons doesn't seem to extreme for a single page.

Test case with a simple view and panel is attached.
Bug 5806 Test Case.zip

Config info:

  • Vaadin version: 13.0.7
  • Chrome version: 74.0.3729.169
  • Java version: openjdk version "11.0.3" 2019-04-16
  • OS version: MacOS 10.14.4
Uncaught RangeError: Maximum call stack size exceeded.
    at vB (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:404)
    at Bv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:760)
    at ew.iw [as yb] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Ju (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:714)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
BFP bug
Source
picture mpilone

Most helpful comment

Thanks for extracting those details. Based on those, the issue is really clear for anyone of the ~3 persons in the world who actually know how those parts of the code are supposed to work.

The client-side processing is really roughly split up in two separate phases: we first update the internal representation of the state, and then we update the DOM to match the updated state. The first phase adds flush listeners which are then run during the second phase.

Processing is split up in this way because in some cases a DOM update is dependent on multiple different state updates and it will either fail or do redundant work if the DOM update is done before all state changes have been applied.

In the case of @Id elements, there's one extra asynchronous step because Polymer must first create the target element and only then can we find it and apply server-side changes to it. This means that the updates for the @Id element cannot be applied during the "master" flush() that happens when the state has been fully updated based on changes from the server. For this reason, an explicit flush() is triggered if the element is processed from the asynchronous ready event from Polymer.

In this case with many @Id elements added as children of another @Id element, processing the parent element causes one flush listener to be added to the queue for each child. It then goes on to do a flush() which picks the first child flush listener from the queue. This triggers a (non-async) Polymer ready event for the child which in turn leads to a new flush(). This flush will pull the flush listener of the second child from the queue and execute it, and so the recursion goes on. If the stack doesn't blow, all child flush listeners will eventually be purged from the queue and each flush() on the stack will see an empty queue and thus return.

I see three alternative ways out of this problem:
1) Schedule this particular flush() in a microtask instead of running it inline. This technically simple and have a very small risk of side effects. On the other hand, it would add additional complexity and asynchronosity to this case which is already too complex and too asynchronous.
2) Add a variant of flush() that is a no-op if run while another flush() is already ongoing and use that variant for this particular case. This adds some complexity in Reactive where it "belongs", but it would put a burden on each caller of flush() to know which variant to use.
3) Change the current flush() to always bail out in case a flush() is already ongoing. This seems safe based on my quick analysis of all (non-test) cases that are calling flush(), but there's no good way to be 100% certain.

@denis-anisimov: What do you think?

picture Legioth on 29 Aug 2019
👍2

All 11 comments

An additional JS stacktrace from the same error:

Uncaught RangeError: Maximum call stack size exceeded.
    at HTMLElement._valueToNodeAttribute (properties-changed.html:480)
    at HTMLElement._propertyToAttribute (properties-changed.html:457)
    at Object.runReflectEffect [as fn] (property-effects.html:388)
    at runEffectsForProperty (property-effects.html:162)
    at runEffects (property-effects.html:128)
    at HTMLElement._propertiesChanged (property-effects.html:1660)
    at HTMLElement._flushProperties (properties-changed.html:341)
    at HTMLElement._flushProperties (property-effects.html:1510)
    at HTMLElement.ready (property-effects.html:1615)
    at HTMLElement.ready (element-mixin.html:609)
    at HTMLElement.ready (vaadin-control-state-mixin.html:96)
    at HTMLElement.ready (vaadin-element-mixin.html:50)
    at HTMLElement.ready (vaadin-button.html:140)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at HTMLElement.connectedCallback (vaadin-control-state-mixin.html:162)
    at HTMLElement._attachDom (element-mixin.html:651)
    at HTMLElement._readyClients (element-mixin.html:624)
    at HTMLElement._flushClients (property-effects.html:1524)
    at HTMLElement.ready (property-effects.html:1619)
    at HTMLElement.ready (element-mixin.html:609)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
    at HTMLElement.s.ready (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at HTMLElement._enableProperties (properties-changed.html:321)
    at HTMLElement.connectedCallback (properties-mixin.html:219)
    at HTMLElement.connectedCallback (element-mixin.html:595)
    at nv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:941)
    at Fv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:902)
    at Gx.Hx [as W] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at eA (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:866)
    at pv (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:976)
    at Zw.$w [as J] (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at Function.yl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:983)
    at d (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:637)
    at Set.forEach (<anonymous>)
    at bl (client-3B1D64116B46DEBA7ADE32AB75416693.cache.js:747)
mpilone picture mpilone on 30 May 2019

Here's an updated test case that removes push and any server side threading. It appears that the issue is reproducible in Chrome with 45 buttons when the buttons are removed and then re-added even if the action is initiated from the client side in a super simple view.

Bug 5806 Test Case.zip

mpilone picture mpilone on 21 Aug 2019

Hi @mpilone

Have you tried to utilize the slot in the panel template to add content?

For example, modifying bug-5806-panel to have the following structure:

  <template>
    <style include="shared-styles">
      :host {
        display: block;
      }
    </style>

    <slot>
      <!-- default slot -->
    </slot>

    <!-- <vaadin-button id="ack-btn"></vaadin-button> -->
  </template>

Then, also updating how the buttons are added in Bug5806Panel:

@Tag("bug-5806-panel")
@HtmlImport("src/ui/view/bug-5806-panel.html")
public class Bug5806Panel extends PolymerTemplate<Bug5806Panel.Bug5806PanelModel> {

    private Button ackBtn;

    public Bug5806Panel(String name) {
        ackBtn = new Button("Button for " + name);
        getElement().appendChild(ackBtn.getElement());
    }

    /**
     * This model binds properties between SystemMonitorActiveAlarmPanel and system-monitor-active-alarm-panel.html
     */
    public interface Bug5806PanelModel extends TemplateModel {
    }
}

allows to add much more than just 50 buttons to the page without any errors on the client side.

gatanaso picture gatanaso on 27 Aug 2019

I'll see if I can do something similar but this seems like more of a workaround than an actual solution. With this approach the button(s) are not visible in the panel design and only the slot is visible. That makes things a little harder on my UI designer who is trying to build out the panel in the Vaadin Designer.

I enabled superdevmode for the test case and this is the stack trace that causes the error in Chrome.

Bug5806_Stacktrace.txt

mpilone picture mpilone on 27 Aug 2019

The root issue looks like recursion in the SimpleElementBindingStrategy.java. I'm still digging into it but you can easily see 42 blocks in the stack trace like:

v3_g$ (SimpleElementBindingStrategy.java:1090)
Z3_g$ (SimpleElementBindingStrategy.java:1044)
J4_g$ (SimpleElementBindingStrategy.java:810)
U7_g$ (SimpleElementBindingStrategy.java:810)
Seb_g$ (Reactive.java:102)
x3_g$ (SimpleElementBindingStrategy.java:910)
r4_g$ (SimpleElementBindingStrategy.java:862)
t6_g$ (SimpleElementBindingStrategy.java:862)
YJ_g$ (PolymerUtils.java:590)
lambda_0_g$ (Runtime.java:166)
$I_g$ (PolymerUtils.java:590)

That is approaching the number of panels (45), and therefore buttons, on the view in the test case. It seems like the call stack will recursively grow based on the number of items on the page.

mpilone picture mpilone on 28 Aug 2019

I don't completely understand this code but the basic recursive call structure is:

  1. Reactive.flush:97
  2. SimpleElementBindingStrategy.handleChildrenSplice:1012
  3. SimpleElementBindingStrategy.addChildren:1055
  4. DomAPI.insertBefore
  5. SimpleElementBindingStrategy.ready:304
  6. PolymerUtils.fireReadyEvent:583
  7. SimpleElementBindingStrategy.addVirtualChild:839
  8. Reactive.flush:97

It looks like a ready listener is added to PolymerUtils to call appendVirtualChild at SimpleElementBindingStrategy.java:887 with reactivePhase=false. This causes the Reactive.flush method to be called recursively at line 910 when the listener is fired. It seems like that is going to greatly reduce scalability (and potentially performance) because the call stack will be directly related to the number of virtual children elements on the page.

mpilone picture mpilone on 28 Aug 2019
👍1

Thanks for extracting those details. Based on those, the issue is really clear for anyone of the ~3 persons in the world who actually know how those parts of the code are supposed to work.

The client-side processing is really roughly split up in two separate phases: we first update the internal representation of the state, and then we update the DOM to match the updated state. The first phase adds flush listeners which are then run during the second phase.

Processing is split up in this way because in some cases a DOM update is dependent on multiple different state updates and it will either fail or do redundant work if the DOM update is done before all state changes have been applied.

In the case of @Id elements, there's one extra asynchronous step because Polymer must first create the target element and only then can we find it and apply server-side changes to it. This means that the updates for the @Id element cannot be applied during the "master" flush() that happens when the state has been fully updated based on changes from the server. For this reason, an explicit flush() is triggered if the element is processed from the asynchronous ready event from Polymer.

In this case with many @Id elements added as children of another @Id element, processing the parent element causes one flush listener to be added to the queue for each child. It then goes on to do a flush() which picks the first child flush listener from the queue. This triggers a (non-async) Polymer ready event for the child which in turn leads to a new flush(). This flush will pull the flush listener of the second child from the queue and execute it, and so the recursion goes on. If the stack doesn't blow, all child flush listeners will eventually be purged from the queue and each flush() on the stack will see an empty queue and thus return.

I see three alternative ways out of this problem:
1) Schedule this particular flush() in a microtask instead of running it inline. This technically simple and have a very small risk of side effects. On the other hand, it would add additional complexity and asynchronosity to this case which is already too complex and too asynchronous.
2) Add a variant of flush() that is a no-op if run while another flush() is already ongoing and use that variant for this particular case. This adds some complexity in Reactive where it "belongs", but it would put a burden on each caller of flush() to know which variant to use.
3) Change the current flush() to always bail out in case a flush() is already ongoing. This seems safe based on my quick analysis of all (non-test) cases that are calling flush(), but there's no good way to be 100% certain.

@denis-anisimov: What do you think?

Legioth picture Legioth on 29 Aug 2019
👍2

I agree that the simplest way to fix this is to change 

Reactive.flush();

to 

Scheduler.get().scheduleDeferred(()-> Reactive.flush());

But that's from the main code of view.
I have a suspicion that Gwt HTML unit tests will fail with this change and the one who is fixing this will need to mock Scheduler. That's doable and we have such mocks already in those tests somewhere but that's requires additional work.

I think we should avoid an additional/ overloaded flush method since it will complicate Reactive "API" which already is quite complicated: you will need to understand in which case you should call flush and in which call another variant of it based on the usecase which is far from obvious.

I think flush should be modified that it doesn't do anything if it's inside of stacktrace of another flush (flush is not completed yet and another flush is called).
I agree with @Legioth that it should not cause any issues (but of course we may not be sure for 100% without running the existing tests).

I believe changes in the flush method is the simplest way in the end. I would recommend go with this approach.

denis-anisimov picture denis-anisimov on 29 Aug 2019

The fix for this will require for sure:

  • some Gwt unit tests (or just unit tests if possible)
  • IT test for the usecase in this ticket.
denis-anisimov picture denis-anisimov on 29 Aug 2019

Thanks for the follow up and the detailed explanation of the issue. If anyone else comes across this issue, I implemented a workaround with a custom component that will add 25 children items at a time to keep the call stack depth in check. It is obviously slower than doing it all in one request but it allows all the items to eventually be rendered.

partitioned-component-adder.zip

mpilone picture mpilone on 29 Aug 2019

@Legioth @denis-anisimov In order to see if the proposed method will break some existing regression tests I issued pull requests. The build was green.

TatuLund picture TatuLund on 25 Sep 2019
Was this page helpful?
0 / 5 - 0 ratings

Related issues

SomeoneToIgnore picture SomeoneToIgnore  路  4Comments
vlukashov picture vlukashov  路  3Comments
ggecy picture ggecy  路  3Comments
SomeoneToIgnore picture SomeoneToIgnore  路  4Comments
appreciated picture appreciated  路  3Comments