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

[GoogleCodeExporter]

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 reported on code.google.com by google...@pcforum.hu on 19 Jun 2009 at 4:35

[GoogleCodeExporter]

Test suite attached.

Original comment by google...@pcforum.hu on 19 Jun 2009 at 5:16

Attachments:

• SliderBug.page
• SliderBug.php

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 20 Jun 2009 at 1:50

[GoogleCodeExporter]

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.

Original comment by Christophe.Boulain@gmail.com on 19 Jul 2009 at 4:33

• Changed state: Accepted
• Added labels: Milestone-3.2a
• Removed labels: Milestone-3.1.6

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Events are now removed and added for every postback/callback

Original comment by uaca...@gmail.com on 10 Mar 2011 at 8:58

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 12 Jul 2011 at 9:18

[GoogleCodeExporter]

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?

Original comment by uaca...@gmail.com on 13 Jul 2011 at 2:32

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 10:15

Attachments:

• prado-r28888-regression-testcase.zip

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 10:32

Attachments:

• prado-r28888-regression-testcase-2.zip

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 10:56

[GoogleCodeExporter]

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.

Original comment by uaca...@gmail.com on 13 Jul 2011 at 1:03

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 1:47

[GoogleCodeExporter]

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.

Original comment by uaca...@gmail.com on 13 Jul 2011 at 2:18

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 2:53

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 13 Jul 2011 at 3:38

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 13 Jul 2011 at 4:33

[GoogleCodeExporter]

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);
}

Original comment by cezarpirajant@gmail.com on 13 Sep 2011 at 2:52

[GoogleCodeExporter]

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

Original comment by ctrlal...@gmail.com on 13 Sep 2011 at 3:37

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 2 Mar 2012 at 3:42

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

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.

Original comment by google...@pcforum.hu on 11 Mar 2012 at 11:18

Attachments:

• prado-client-side-registry-fix-with-timers-4.zip

[GoogleCodeExporter]

patch committed as r3117

Original comment by ctrlal...@gmail.com on 24 Mar 2012 at 9:30

[GoogleCodeExporter]

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

Original comment by google...@pcforum.hu on 11 May 2012 at 3:23

[GoogleCodeExporter]

patched in r3139, thank you

Original comment by ctrlal...@gmail.com on 11 May 2012 at 3:52