Prado client-side flawed - aka Multiple client-side (JS) object instances created for the same single DOM control

[ctrlaltca]

From google...@pcforum.hu on June 19, 2009 18:35:23

The current implementation of several JS-supported non-active controls and/or of TClientScriptManager::registerPostBackControl() interferes with the active control / callback mechanism in Prado, as it doesn't care about the type of the actual page cycle and the state of the control. The problem is that nor registerPostBackControl() neither its callers care about whether the actual page-cycle is a full-page reload or just a callback, and they (actually registerPostBackControl()) just simply always emit the JavaScript code which is responsible for the creation of the client-side JS objects for several JS-supported controls ("new Prado.WebUI.XXX()" lines at the end of the page). This is OK at initial page loads, but if this happens in a callback request, it leads to the creation of a new, redundant JavaScript client-side objects for controls that already might have one (or more) JS client-side object instantiated for them, thus leading to multiple (possibly conflicting) client-side JS support objects living on the page for the affected controls. This in turn results in very odd and buggy behaviour on the client side, and might also seriously effect the server running the Prado application.

In the worst case this leads to doubling the number of the actual client- side JS objects for every affected control, which not only makes the browser eat more and more memory after each callback, but can also make your server crawl and lead to unpredictable behaviour if any of the affected controls do callbacks themselves too, (either in response to user actions or due to timed callbacks and such), because now every one of those client-side JS objects will fire their own callbacks for that single Prado control. After 8 callbacks you will have 256 client-side JS objects for every single one of the affected DOM controls in the page!

For ex. if you create an "active version" of TSlider and hook into its onChange event, where you initiate a callback back to your page, so you get notified when the user dragged the control, then - after executing your callback -Prado will send back a response to the client-side with a new "new Prado.WebUI.TSlider({'ID':...)" JavaScript block in it. This happens because TSlider calls TClientScriptManager::registerPostBackControl () from its onPreRender() method which gets executed at every page cycle (even at callbacks) and because registerPostBackControl() doesn't care either whether the slider is a newly created control (in which case it would be legitimate to create a client side TSlider JS object for it) or an already existing one (which already has a JS object created, up and running for it in the browser), but simply emits the code for the client- side TSlider Js-object creation ("new Prado.WebUI.TSlider...) again, thus creating a second JS object instance for the very same object that not only already has one, but also was the control that initiated the callback cycle in the first place. Now, if you drag the slider, there will be 2 JS- side client objects monitoring and handling the control, even though that won't be apparent to you, because they all do the same thing. But when you release the slider, both of them will fire their onChange() events, thus resulting in not only one, but two callbacks to the server code - both returning a response which will create another two JS instances for the slider, thus resulting in 4 (!) JS objects handling the slider in total. If you drag the slider again, you will have 8, 16, 32, 64... JS-objects which all hook up to the same single slider control both on the page and also on the server side.

Other controls might exhibit different behaviour due to the redundant objects and might lead to very weird effects. For ex. a control alternating between two states might become unable to be change it's state after the first change, as it - well, actually the JS objects linked to it - will always "see" an even number of events (2,4,8,16...) fired on it, thus resulting in the reversal of any attempt of change back to the original state. Controls which have a single well defined response and take a well-defined state for each user action, won't be crippled by the effects of this bug, because all their linked object-instances would send them into a state anyway, into which the first one sends them, thus resulting in no apparent glitch. For ex. even though TTabControl is also affected by this bug, it doesn't show any glitches, as no matter how many JS objects listen to the control's events, all of them will select the same tab for a click at a specific set of coordinates, and if the tab is already selected, then that won't result in any error either, so you won't even notice that there are now for ex. 32 client-side TTabControl objects handling your clicks on that single tab control in the DOM.

OK, so much about the problem. Now, the solution. The bad new is: there isn't a simple one. A complete solution would require to extend TClientScriptManager::registerPostBackControl() (or it's callers) to keeps track of each control's state and only emit the JS code for them if either the current page cycle is an initial full page-load, the control gets replaced in the DOM by a new instance (because for ex. it was on a TActivePanel which got refreshed, or because it was an active control itself which got it's properties modified), or the control is a newly created one which has been constructed in the callback. If none of these are true, it should withhold the code for creating the client-side JS object for the control as there's already one of those at work in the browser. Implementing that will be though to, because of having to keep track of all the updates.

Original issue: http://code.google.com/p/prado3/issues/detail?id=181

[ctrlaltca]

From google...@pcforum.hu on June 19, 2009 10:16:17

Test suite attached.

Attachment: SliderBug.page SliderBug.php

[ctrlaltca]

From google...@pcforum.hu on June 20, 2009 06:50:15

OK I just got a simpler idead to fix the issue. That is: there should be a client- side registry these JavaScript-supported controls, and the JS-objects themselves should be modified so they register themselves and the events they want to listen to in that registry. Now if there's a registration for the same control, the registry should first dispose of the old object for that control - if there's any already - and register the new object as the handler for the control and for the future. It is important tho that when deregistering, the object should release all event handlers and such too it hooked when it was created. It is also important that all controls preserve their state information in the DOM (instead of in the JavaScript object) or that when a replacement in the registry occours, all state fields of the previous object get copied to the new one (they should have a method for this in that case), so that the control's state information doesn't get lost.

I'm also thinking about whether this issue with multiple instances does affect other controls too, which render "Event.observe()" snippets in their OnPreRender() methods using registerEndScript(). That might lead to multiple events (event handlers) fired for a single event on a single control too, after one or more callbacks have been executed on the page, but I will still have to check that.

Anyway, it would be good to hear something from the Prado core developers about this issue, as it is a really serious design flaw I think, which could affect practically all Prado applications and lead to very weird and seemingly untraceable bugs, therefore should be fixed ASAP.

[ctrlaltca]

From Christophe.Boulain@gmail.com on July 19, 2009 09:33:07

Thanks for your detailled report. I'm not really a javascript expert, so, I'm not sure how I can fix this issue. We already have a client side registry since 3.1.5, so, we may implement your idea in it.

It would be great if you have some code to provide to help us to fix this issue.

Anyway, I move this issue to 3.2. If we can find a good solution, we may backport it to 3.1.

Status: Accepted
Labels: -Milestone-3.1.6 Milestone-3.2a

[ctrlaltca]

From uaca...@gmail.com on March 10, 2011 12:58:04

Events are now removed and added for every postback/callback

Owner: uacaman

[ctrlaltca]

From google...@pcforum.hu on July 12, 2011 14:18:01

This patch causes regression in 3rd party components that

1. are derived from TActiveControl
2. use "Event.Observe(...)" calls to install event handler(s)
3. the handler codes are emitted/registered either with THtmlWriter::write() or TClientScripManager::registerEndScript() calls
4. the latter are done so prior to calling the TControl's render() method (like for ex. in OnPreRender(), or in the control's render() method, but before the call to the parent::render), or if overridden, prior to the TActiveControlAdapter::registerCallbackClientScript() or TClientScriptManager::registerCallbackControl() call.

In these controls the extra event handlers won't be actually ever called, and thus some or all functionality of the controls will be lost.

This is because the new Prado.WebUI.PostBackControl.initialize() method unbinds ALL existing event handlers, therefore disabling all existing bindings for the control it has been created for. So if the 3rd party component has registered it's handlers prior to the construction of the client side object, those event handlers will be all discarded. This didn't happen prior to r2888 , but does now.

The solution is to postpone event handler registration after the construction of the client-side object (ie. emit the event handler bind code after the above mentioned calls, preferably to the end of the control's ::render() method), or to subclass the client-side object itself, and move all event registration code into the newly created control class. (The latter is, however, tricky as with JavaScript/prorotype there's no straighforward way to override the initialize/onInit() method and still call the parent implementation).

[ctrlaltca]

From uaca...@gmail.com on July 12, 2011 19:32:59

you mean the users controls that are derived from TActiveControl? If so those controls mus register the events again or the events will fire as many times as the control is created.

Do you have a example of this behavior?

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 03:15:52

No, I mean the regression occours when all 4 above mentioned conditions are met. It holds also true for any application-level code that meet criteria 2-4. and tries to install event handlers on controls that meet criteria 1.

I've attached a simple test case. Try to run that with and without r2888 applied to the framework!

A framework-level - and much cleaner (in my opinion) even though less simpler - solution would be to leave it up to the client-side objects deregister their own handlers (instead of purging all handlers unconditionally), as that would preserve all external handlers installed by either other controls or by application-level code.

Attachment: prado-r28888-regression-testcase.zip

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 03:32:12

There's another issue with patch r2888 , and that is that it only takes care of event handlers installed inside the prototype/Prado framework but does not consider hooks and callbacks outside of those. However, there might be very well other kinds of hooks and callbacks active and installed, either for ex. in third part libraries or even just as simple things as outstanding periodic timers (windows.setTimeout()).

See attached test case how patch r2888 does't solve duplicate registration issues with controls that have also timers installed.

As already described above, the solution to all these would be to

1. define a "deinitialize" method in the client-side base control class
2. the client-side control registry should call that method on the previous instance of the client-side wrapper when a control object with the same ID is being constructed/registered
3. each client-side control should override said "deinitialize" method and use it to deinstall all their event handlers (meaning explicitly and only those installed by them), stop all timers, and release all other resources they've allocated

This would solve both the regression and the issue with the timers and other possible callbacks.

Attachment: prado-r28888-regression-testcase-2.zip

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 03:56:37

Sorry, I just realized that even the above described method wouldn't solve all problems, as for ex. when the timer control would be a dinamically created one, and would be removed (or not re-instantiated) in a refresh cycle, that would still leave a dandling client-side object for it on the page. That would happen because in this scenario there would be no attempt to re-register a control with the same ID, so de-registration of the already existing client-side object instance wouldn't happen either - where in fact the latter should happen, since now the host control have been removed from the control tree on the server side and doesn't exists anymore. So, in fact, since the client-side object for the timer would be still there, it would still try to issue the scheduled callback - but would fail at that and would raise a server-side exception (probably periodically), as Prado wouldn't be able to find the host control for the callback.

So, I think the only really perfect solution would be the one described in my original comment, ie. keeping track of all object disposals and creations, and manage client-side wrapper disposal from the server-side. That, however, would be fairly complex and consume probably a lot of resources. So, I'd say realistically we should settle for the non-perfect solution described in comment #10, which would still take care of 99% of the cases.

There's also another solution to the problem, which however is more like a workaround. The solution could be that 1. all controls should emit their markup with at least one html element having a distinctive and unique ID (this now happens only if the ClientID property of the control has been referenced prior to rendering), 2. even controls that basically don't need any HTML markup (like the TTimeTriggeredCallback) would need emit some "dummy" code (like a hidden div). Then, after each callback all registered active controls should be asked by the framework/client-side registry (by the means of a call to a method defined for that purpose in the base client-side wrapper) to check for the existence of their markup counterpart in the DOM (or for that dummy div in the case where basically no markup is needed), by looking for that specific unique ID. If they can't find their markup in the DOM that means that their host controls have been eliminated from the control tree during the last callback/refresh cycle - and in this case the objects should just "commit suicide", and disable themselves. That would solve the situation with the lingering client-side control objects described above - but without the heavy burden to have to keep track of each and all control's lifecycle on the server side, and communicate that to the client-side control manager.

[ctrlaltca]

From uaca...@gmail.com on July 13, 2011 06:03:45

As of late i have been getting some issues where i need control over a control created in the client side, simple stuff like update the value of a slider or call a event on the server without recreate the whole repeater from database to just to give the user a warning about delete a item. Giving it some thought i think the problem is that PRADO keeps track of "static" controls in the template but does not keep track of dynamic controls. If we do have someway of keep tracking of all the server controls i guess it would be necessary to reflect the update and destroy of those controls on the client side and bamm, problem goes away. This also gives a few other benefits, for example, today it not possible to have a TActiveMultiView simple because the necessary javascript for a control is created ONLY at postback and event if a control is created at callback it wont work on the client side. As you described above this is a consume a lot of resources and would not be backwards compatible.

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 06:47:20

I have actually created a TActiveTabPanel which only renders a single tab at all times (but of course depending on which tab you click, it will be a different one). So that's not a problem. Also controls created inside callbacks render fine and have an appropriate client-side wrapper created, too. (The trick is that you have to keep track of those dynamically added controls in the pagestate, and recreate them prior to letting Prado handle the callback, or it won't find the callback target.)

So, the problem isn't with dynamically created controls (even though it's PITA to keep track of them in the pagestate and recreate them in every callback, but maybe there could be some help from the framework to do that). The problem is with updating and deleting already existing controls.

[ctrlaltca]

From uaca...@gmail.com on July 13, 2011 07:18:54

Well i did a TActiveTabPanel myself, try to add a validator to it, it will only work if you have a validator outside of the TActiveTabPanel or in the default view. Every time the page renders validator manager try to register the validator with the form, it there is none validator wont work anymore even you you create it in the callback, that is true for a few others controls. Problem is that validator dont register the javascript source if there is not at least one visible validator.

What i think that should be done is every time a control is created, updated or made invisible on the server it should let the client side know and let it handle the event registration.

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 07:53:11

That sounds like a different issue. The validation framework has nothing to do with the client-side control registry per se. It's using a different registry, with obviously having with its own issues.

Another issue I'm aware of is however, that the registration of new client-side scripts won't be taken care of in callbacks either. That means that if you try to instantiate a control in a callback that uses and registers a .js file otherwise properly (with TClientScriptManager::registerScriptFile() or ~::registerPradoScript()), that isn't included/referenced on the original page already, Prado won't load the additional .js file(s) when processing the callback response either, thus preventing the control from functioning properly.

This issue can be worked around by including a hidden instance of the control class(es) in question on the original, always-visible porition of the page, in which case the controls instantiated in the callbacks will function properly. This however is is an ugly hack, and also might waste bandwidth and cause loading delays in cases where you have a lot of controls which can be instantiated in callbacks, but most of the time most of them won't actually ever get to appear on the page during the whole life cycle of the latter.

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 08:38:44

Created a new ticket ( issue 347 ) for the validation problem which is distinct from this one, and has to do rather with missing registration than with duplicate/multiple registrations. (And it's an issue with the validation registry, not with the control registry.)

[ctrlaltca]

From google...@pcforum.hu on July 13, 2011 09:33:26

Fixed your validation problem. See issue 347 . Validation now seems to be working flawlessly in my TActiveTabPanel control, even on initially non-visible tab pages. If you still have problems in yours then please supply a test case in issue 347 .

[ctrlaltca]

From cezarpirajant@gmail.com on September 13, 2011 07:52:48

I believe there is a problem in revision r2888 of the inlineeditor.js file.

On line 33 I realized that the code was added

/ / Issue 181 $ (this.options.ExternalControl). stopObserving ('click', this.onclickListener);

But it took the old line 33 out of the IF.

In other words, the lines 33 to 35 should not be?

if (this.options.ExternalControl) { / / Issue 181 $ (this.options.ExternalControl). stopObserving ('click', this.onclickListener); Event.observe ($ (this.options.ExternalControl), 'click', this.onclickListener); }

[ctrlaltca]

From ctrlal...@gmail.com on September 13, 2011 08:37:47

applied the patch to inlineeditor.js in r3038 , thank you

[ctrlaltca]

From google...@pcforum.hu on March 02, 2012 07:42:18

This issue is still marked as fixed even though the testcase attached to my comment #10 is still successfully exposing the very basic flaw of patch r2888 , even wit the latest trunk sources. Also the problem with killing all event handlers attached to the control wrapper, but not related directly to the creation of the latter, is still present, as described in comment #7.

As already explained above the only proper solution that would solve all such issues once and for all would be to add some kind of standard deinitialization procedure to the Prado client-side control wrappers, which should be trusted to remove the event handlers installed by them (and only those) whenever the controls the wrappers are representing get replaced during a callback.

This would 1. prevent having multiple client-side wrappers lingering around attached and/or operating on the same control(s), 2. eliminate all problems arising from automatic and unconditional detachment of all (also external) event handlers whenever recreating a control wrappers (which is what r2888 is doing).

[ctrlaltca]

From google...@pcforum.hu on March 11, 2012 16:18:16

I've created a patch to fix both issues - and leave room for even more flexible client-side control management.

The key changes are:

1. there's now a new Prado.WebUI.Control class which is supposed to be the base of all client-side control wrappers. This class automatically detects whether a wrapper is being instantiated "over" a previous wrapper with the same ID, and if that's the case, it makes sure that the previous wrapper gets properly disposed of, before firing up the new wrapper. Wrappers are now supposed to properly clean up after themselves, including - but not limited to - disposing of all their event handlers. This makes sure that there will be no lingering event handlers left when recreating a wrapper, but also makes sure that any event handlers installed not directly by the wrapper stay intact (which was not the case since r2888 ).
2. instead of the original Event.observe() all wrappers use now Prado.WebUI.Control's observe() method to install their event handlers. The latter method keeps track of all the registered event handlers, and disposes of them when the wrapper gets destroyed (because of a new wrapper replacing it - see above). This simple extension takes out the pain of deinstalling event handlers when destroying the wrapper, while requiring only minimal modifications to any existing wrappers (ie. you only have to replace "Event.observe()" calls with "this.observer()" class). Event observing can be stopped with a call to the this.stopObserving() method.
3. similar changes implemented for timers (both periodic and one-time): instead of window.setTimeout() and window.setInterval the controls are calling this.setTimeout() and this.setInternal(). When the timeout is over, a wrapper function checks whether the control wrapper is still alive, and only fires the timer event if it is; for periodic timer events (ie. setInterval), it automatically cancels the timer upon deinitialization of the object. That is, if it hasn't been already cancelled with a call to this.clearInterval().
4. I've also incorporated changes to take advantage of the new class inheritance syntax introduced in Prototype 1.6, and to make all class initializations look uniform

I didn't touch some client wrapper classes (like datepicker.js, ratings.js) which will be therefore still prone to the two existing bugs described above. I left them untouched because I'm not using them and didn't want to build projects to test them. However, I see no problem with applying the same changes to them (ie. make them descendants of the new base control class, remove their custom initialization code, and make them use automatically detached observers).

I've tested most of the other controls, however, peer review wouldn't hurt them either - even though their core behavioral codebase hasn't been touched at all.

Attachment: prado-client-side-registry-fix-with-timers-4.zip

[ctrlaltca]

From ctrlal...@gmail.com on March 24, 2012 14:30:14

patch committed as r3117

[ctrlaltca]

From google...@pcforum.hu on May 11, 2012 08:23:38

This patch needs a correction. In TTimerTriggerCallback::setInterval() the line

$client->callClientFunction('Prado.WebUI.TTimeTriggeredCallback.setInterval', array($this, $interval));

should be changed to

$client->callClientFunction('Prado.WebUI.TTimeTriggeredCallback.setTimerInterval', array($this, $interval));

to reflect changes in the client-side class method naming of r3117 . Otherwise the timer interval will not be updateable from callbacks and every timed callback will raise an exception in the browser upon return, due to the setInterval method references in the PHP code not being existent in the client wrapper JS class. However, this exception will only be obvious if running the browser in the debug mode or with the JavaScript console enabled, as otherwise the exception will be supressed ("eaten") in Prado.CallbackRequest::__run().

[ctrlaltca]

From ctrlal...@gmail.com on May 11, 2012 08:52:13

patched in r3139 , thank you