Comment 1 Ivan Furnadjiev 2012-02-10 03:47:25 EST

Cole, I can't imagine a use case of this. Browser#execute is a blocking operation that waits for a result from the script. Calling Browser#execute twice, one after another is working fine. Sure, you could call Browser#execute from asyncExec when another execute operation is pending and currently this is not possible. I think that having more than one blocking operation at time will cause a lot of trouble.

Comment 2 Ralf Sternberg 2012-02-10 05:33:56 EST

Cole, it's possible to return arrays from an executed function. This should allow to combine several functions into a single one. Since every execute call results in a synchronous turnaround, I would rather recommend going this way than sending out several executes in sequence. Would that help in your case?

Comment 3 Cole Markham 2012-02-13 12:20:51 EST

I have a patch that is working well. The current tests are passing, but I'm having trouble writing a new test. I'm putting together a snippet project to demonstrate the use case. The main idea is that a background thread is pushing updates to a browser widget via asyncExec. This could be something like a system log or a chat client. If Browser#execute returns false, you can't determine whether it failed on the browser or was discarded because another execute was pending. Therefore the client code doesn't know whether to retry the execution. I'll provide some more examples in my snippet, but I think it makes more sense for the Browser widget to handle this. I haven't looked at SWT to see if the same issues exist there, but I will probably file a bug there as well.

Comment 4 Cole Markham 2012-02-27 12:30:21 EST

Created attachment 211676 [details]
Patch to implement server-side queuing of calls to browser#execute

Comment 5 Cole Markham 2012-02-27 12:31:05 EST

Created attachment 211677 [details]
Patch for test cases

Comment 6 Tim Buschtoens 2012-02-28 05:58:58 EST

Thanks for the patch. I think i now get what the problem is. As far as i can tell the patch would bring pretty huge refactorings to HEAD, and i'm feeling uneasy making those so late in the release cycle - especially to a (at least to me) very complicated widget for a relatively rare use case. I would suggest to wait till after the 1.5 release before trying to fix this issue.

Comment 7 Cole Markham 2012-04-19 14:05:24 EDT

It looks like a different version of this patch was applied to CVS head?

Comment 8 Ivan Furnadjiev 2012-04-19 14:15:02 EDT

(In reply to comment #7)
> It looks like a different version of this patch was applied to CVS head?
Why do you think so? The recent change to the Browser widget was a fix for bug 375352.

Comment 9 Cole Markham 2012-04-19 14:18:39 EDT

Ok, that makes sense now. That was half of what this patch did. I'll rewrite my patch for CVS head.

Comment 10 Cole Markham 2012-04-19 17:39:31 EDT

Created attachment 214280 [details]
Updated patch

I updated my fix to work with Ivan's new support for asynchronous callback. All test cases pass. This is a less drastic change than my previous pass since Ivan implemented half of what I was trying to do with his fix for bug 213737. With the current push to move 3rd party Javascript integration from custom RAP widgets to use the Browser, I think this will become an issue for more users and it will be important to have in 1.5.

Comment 11 Gábor Lipták 2014-01-29 04:41:22 EST

Hi,

I guess this is more serious problem than expected. In our application we have tabs. The tabs may contain NVD3 charts. I implemented a ControlListener, which resizes the NVD3 chart if the embedding Browser component is resized. This works perfectly. BUT. If I open up a new tab which is opened next to my existing tab (so that it becomes resized), then the whole application completely freezes, and in the javascript console I see the classical "another browser function is still pending" message.

First I thought I made a mistake, and I call a browser execute or evaluate from a BrowserFunction like it is described at bug 371132 (https://bugs.eclipse.org/bugs/show_bug.cgi?id=371132). After debugging around it turned out, that the following happens:

1. I open the first tab. RWT Browser "A" is containing NVD3 chart. Everything is fine.
1. I open the second tab
2. The RWT browser "B" of the second tab starts to evaluate its javascript
3. Since execute method of the RWT browser "B" calls dispatch on display events, my resize event in the other tab starts running, and tries to evaluate a javascript in the other RWT browser "A" instance. This will fail, and the whole RAP client starts to hang.

So the problematic code is the following:

  public boolean execute( String script ) {
    checkOperationMode();
    checkWidget();
    if( script == null ) {
      SWT.error( SWT.ERROR_NULL_ARGUMENT );
    }
    boolean result = false;
    if( executeScript == null ) {
      executeScript = script;
      executeResult = null;
      while( executeResult == null ) {
        Display display = getDisplay();
        if( !display.readAndDispatch() )  {
          display.sleep();
        }
      }
      executeScript = null;
      executePending = false;
      result = executeResult.booleanValue();
    }
    return result;
  }

I think the whole evaluation of javascripts must be asynchronous, and should be run sequentially. Or I have no idea :)

What do you think guys?

Comment 12 Gábor Lipták 2014-01-29 04:46:00 EST

I have one idea: I could check in my resize listener if any other browser is evaluating code right now (with some locking mechanism), and if yes, I could also call readAndDispatch(). This should work, I just have to pray, that my stack will be large enough :)

Comment 13 Gábor Lipták 2014-02-04 03:07:00 EST

Actually the readAndDispatch resulted in frozen RAP client. So it did not help. To make the problem more general, the problem always happen if you have:
- Browser A has a javascript event calling a BrowserFunction
- Any other Browser or Browser A itself evaluates a script in the Browser component

So having this error is really simple with multiple Browser Widget instances in the same application.

The javascript errors I get are attached as a picture. RAP 2.2 is used.

Comment 14 Gábor Lipták 2014-02-04 03:09:20 EST

Created attachment 239598 [details]
Javascript console text

Javascript console for the pending BrowserFunction which causes the RAP client to die.

Comment 15 Cole Markham 2014-02-05 22:09:48 EST

Gábor, see bug 371132, it seems to better describe what you are experiencing. IIRC, the patch attached here fixes both bugs though. I'll try to find some time to investigate and port the patch to head.

Comment 16 Gábor Lipták 2014-02-06 05:43:52 EST

Hi,

I tried to apply the latest patch against the 2.2-maintenance branch. I attach the patch for you guys.

The patch actually did not help in my case, where there are multiple Browser Widgets, and the BrowserFunction evaluation of Browser A blocks the execute function of Browser B.

I think there must be one queue for all Browser instances in a RAP session. The queue must be shared, and must contain scripts with reference to the Browser instance. It seems that there is also some mutual exlusion for javascript code evaluation on the javascript side.

So actually I have to say that to solve this issue we would need deeper understanding of the Javascript-Java communication and RAP lifecylce, and some guidelines from the RAP Team.

Regards,

Gábor Lipták

Comment 17 Gábor Lipták 2014-02-06 05:44:52 EST

Created attachment 239695 [details]
The patch of Cole applied against the 2.2-maintenance branch.

Comment 18 Gábor Lipták 2014-02-10 12:01:49 EST

We had actually Browser.execute() calls which were executing BrowserFunctions (not the other way around, so no BrowserFunctions calling Browser.execute()). I removed all of these, and with a small trick I made one of them working without problems. I have a BrowserFunction which logs JS errors on server side. I call this using "setTimeout" asynchronously like this:


window.logToServerAsynchronously= function(logLevel,message) {
	window.setTimeout( function() {
		myLoggingBrowserFunctionName( logLevel, message );
	},100);
}

And now I can execute in the browser code like this:

try {
    eval("somestuff");
}
catch(error) {
    window.logToServerAsynchronously("ERROR",error.message);
}

Now I don't have problems anymore. Ugly but works.

Comment 19 Tim Buschtoens 2014-05-20 11:05:40 EDT

With recent changed in RAP it has become possible for a user to cause a script execution error simply by clicking very fast. A single request may now contain multiple events of the same type which are all executed. Since Browser.execute contains a readAndDispatch, this may cause an Browser.evalute call from within another Browser.evalute, even without BrowserFunctions in the mix.

For now I added a IllegalStateException (currently in Gerrit) to make this case distinct from actual JavaScript evaluation errors. 

In the long run I think blocking methods like this are just a dead end for RAP. Dialogs have the same problem (probably, I didn't test). Instead of making it even more complicated and error-prone than it already is, I would like to improve BrowserUtil.evalute, the non-blocking, JEE mode compatible way to execute a script. It can currently not run multiple scripts either, but it should be simpler to achieve and result in more stable code.

In the future Browser.execute/Browser.evaluate could be either removed or changed to work non-blocking. SWT comptability would then either go away or solved in another API layer.