SketchUp / api-issue-tracker

Public issue tracker for the SketchUp and LayOut's APIs
https://developer.sketchup.com/
39 stars 10 forks source link

UI::WebDialog set_on_close block is called twice #102

Open DanRathbun opened 6 years ago

DanRathbun commented 6 years ago

SketchUp API Issue - Documentation

The [UI::WebDialog # set_on_close](http://ruby.sketchup.com/UI/WebDialog.html#set_on_close-instance_method) block is called twice.

This sets up a situation where if the coder is not careful to detect which time through the block it is, there can be a recursive run away resulting in a SystemStackError exception with "stack level too deep" message.

A warning note should be added to the documentation for the #set_on_close method.

Coders should use a variable to detect in a conditional if statement, whether the block has not yet been called and only then do the tasks. Inside the conditional statements this variable should be set so that the second time through the tasks are not redone. Especially if other methods are called from the closure block.

See public forum post for an example ... https://forums.sketchup.com/t/detecting-the-exit-button-click-in-a-browser-dialogs-javascript/71599/9


As the UI::WebDialog class is deprecated and development is "closed", it is not expected that this bug will be fixed. A warning is the best we can hope for.

thomthom commented 6 years ago

Can you distill that thread into a small example?

I tried this:

dialog = UI::WebDialog.new
dialog.set_html('Hello World')
dialog.set_on_close {
  puts 'set_on_close '
}
dialog.show

When I close the dialog I see the message only once.

DanRathbun commented 6 years ago

When I run your simple example I see only 1 message. But I see it twice every time I run my more complex code. (I'll look at my code again and see what might be happening. This might the old thinking I'm stopping the JS event when it isn't and both it and the Ruby-side close() are causing the block to be called twice.

It's weird for sure.

DanRathbun commented 6 years ago

Okay, I see what is happening. It was false assumption on my part.

I assumed that when the red X button was clicked that the dialog had already been closed by the time the set_on_close() block was executed. But this is untrue. The window is still visible, and my code checks this and will call @window.close() from inside the closure handler, which is fired by the set_on_close() block and the button callbacks.

Looking at my code more closely, by the time it gets to this point, close() must have already been called upon the window object. So it was the closure handler causing the second execution of the block, when I didn't even need to do a "close window if visible" there "for good measure".

FALSE ALARM. NOT a bug.

But it is a good example of needing to be extremely careful not to cause vicious circular code calls that can result in a stack overflow. Sometimes "for good measure" backfires.


Again, ... just a ...

Documentation Issue

So the docs can stress that the window is actually still visible during the set_on_close block execution, and will not really be destroyed until the block finishes and the method returns.

It should also explicitly warn coders not to call @window.close from inside the set_on_close() block (or any code called from it,) as this can set up a stack overflow.

thomthom commented 6 years ago

It should also explicitly warn coders not to call @window.close from inside the set_on_close() block (or any code called from it,) as this can set up a stack overflow.

Good one. Thanks for tracking that down.

thomthom commented 6 years ago

Logged as SU-40184