Jetty upgrade broke HtmlUnit for window.onerror

[Daniel Kurka]

Hi all,

I spend a considerable amount of time on getting our changes for window.onerror launched in open source:

https://gwt-review.googlesource.com/#/c/gwt/+/18880/

This causes some tests to fail in html unit:

http://build.gwtproject.org/job/gwt/972/testReport/

Note: these tests are already failing but not reported failing with the newer version of HTML unit. As soon as we are starting to trap window.onerror in tests we actually catch the exception and thus fail those 6 test cases.

Things I have tried:

• Place @DoNotRun on the tests, does not work for the JUnit tests since they require all methods to run
• Downgrade HtmlUnit in open source, fails with a noclass def for WebSocket out of Jetty
• Upgrade HmltUnit to latest (2.27), fails to start the testing shell in open source

Trapping window.onerror the default in open source GWT is important and the right way forward, we simply do not have the bandwith to do it right now. A simple way of making it happen would be to roll back the jetty upgrade. We are not going to invest further into this for OS, but would rather see somebody else pick this up. Any takers?

-Daniel

[Colin Alworth]

Back from the conference now (and no more slides to write!), I'm able to start taking a look at this. I've repro'd the basic case that shows up in the error, might have a few questions for you later so I don't walk down paths you've already determined to be dead ends.

Is there any other documentation on this problem, or a github issue?

[Colin Alworth]

Good news is that getting past the failed to start shell issue isn't that bad, mostly transitive dependency changes (man there are times I love maven, mostly when I'm not using it...), but also losing the ability to run tests in IE8, IE11. Not sure that is a great loss.

Bad news is that it doesn't help - the same errors still happen, the same tests still fail.

Patch from current HEAD and logs of failures in GWTTestCaseSetupTearDownTest (the only one I ran as a representative example) are attached if anyone wants to eyeball it quick, see if I did anything too dumb.

With that out of the way, I'll try to understand the root issue, see if I can't figure out what has changed in HtmlUnit that causes this. Failing that, I'll roll back to the older jetty/htmlunit, and hopefully that will jog something loose in my head.

[Colin Alworth]

Okay, I'm about 80% sure that I understand and can remedy the problem within HtmlUnit itself. Will update once I finish syncing the apparently canonical SVN repo to git, so I can go over the history more carefully and ensure that this break isn't deliberate.

A question for my fellow GWT maintainers: should I patch the version of HtmlUnit that we presently use, so that we avoid breaking downstream expectations (plugins that specify specific browsers, use if IE8 or IE11 as a user agent to run the tests as)? Or should I see about getting this patch into the next version of HtmlUnit, and we wait on their release cycle, and leave this broken until then?

[Thomas Broyer]

On Friday, October 6, 2017 at 7:55:15 PM UTC+2, Colin Alworth wrote:

Okay, I'm about 80% sure that I understand and can remedy the problem within HtmlUnit itself. Will update once I finish syncing the apparently canonical SVN repo to git, so I can go over the history more carefully and ensure that this break isn't deliberate.

A question for my fellow GWT maintainers: should I patch the version of HtmlUnit that we presently use, so that we avoid breaking downstream expectations (plugins that specify specific browsers, use if IE8 or IE11 as a user agent to run the tests as)? Or should I see about getting this patch into the next version of HtmlUnit, and we wait on their release cycle, and leave this broken until then?

I'd say it depends on their release cycle; but we could bundle a patched HtmlUnit into gwt-user (like we bundle a patched JDT into gwt-dev) to get the fix early without waiting for the next HtmlUnit release (assuming the patch would get merged upstream!)

If you (Daniel & Colin) think we should get the fix in 2.8.2, then let's do this (bundle a patched HtmlUnit into gwt-user).

Note that we could possibly just bundle the patched class, and not the whole jar(s)!

[Colin Alworth]

It was not my intention to get this into 2.8.2, but to wait for 2.9. If we think it is important enough, I can look into it, but I figure we have to draw the line somewhere...

[Thomas Broyer]

On Friday, October 6, 2017 at 7:55:15 PM UTC+2, Colin Alworth wrote:

Okay, I'm about 80% sure that I understand and can remedy the problem within HtmlUnit itself. Will update once I finish syncing the apparently canonical SVN repo to git, so I can go over the history more carefully and ensure that this break isn't deliberate.

Ah, I think I understand too!

In 2.13 (before we updated), only the line number was passed to onerror: https://sourceforge.net/p/htmlunit/code/HEAD/tree/tags/HtmlUnit-2.13/src/main/java/com/gargoylesoftware/htmlunit/javascript/host/Window.java#l1238

In 2.14, they started passing the column number: https://sourceforge.net/p/htmlunit/code/HEAD/tree/tags/HtmlUnit-2.14/src/main/java/com/gargoylesoftware/htmlunit/javascript/host/Window.java#l1253

In 2.16, they started passing the exception: https://sourceforge.net/p/htmlunit/code/HEAD/tree/tags/HtmlUnit-2.16/src/main/java/com/gargoylesoftware/htmlunit/javascript/host/Window.java#l1284

But note that this is the ScriptException, not a JS exception. This is possibly where Rhino's Context.javaToJS() is missing, as warned in the logs; or more accurately, the JS exception should probably be extracted out of the ScriptException (if (e.getCause() instanceof JavaScriptException) { args[4] = ((JavaScriptException) e).getValue(); }, see https://sourceforge.net/p/htmlunit/code/HEAD/tree/tags/HtmlUnit-2.19/src/main/java/com/gargoylesoftware/htmlunit/ScriptException.java#l138 for inspiration)

I traced that last change to https://sourceforge.net/p/htmlunit/code/9587/, made without a test…

[Colin Alworth]

Exactly - I wasn't planning on adding the javaToJs(), but was going to unwrap the exception before calling onerror (or have ScriptException implement Scriptable). Have a short test that demonstrates the issue without gwt (but wow they have a lot of GWT in their source tree), and am was waiting for my svn->git sync to finish to confirm what you did by hand. Just in case we decide to rebase it along, or make other improvements down the road...

[Thomas Broyer]

On Saturday, October 7, 2017 at 3:01:52 PM UTC+2, Colin Alworth wrote:

Exactly - I wasn't planning on adding the javaToJs(), but was going to unwrap the exception before calling onerror (or have ScriptException implement Scriptable). Have a short test that demonstrates the issue without gwt (but wow they have a lot of GWT in their source tree), and am was waiting for my svn->git sync to finish to confirm what you did by hand. Just in case we decide to rebase it along, or make other improvements down the road...

I wonder if we couldn't workaround the issue with a custom JavaScriptEngine's handleJavaScriptException? (override it with the same code except with the triggerOnError call "inlined" with the "fix")

This possibly would allow us to have Daniel's change in 2.8.2, without the need to update or locally-patch HtmlUnit at all (and then wait for the fixed HtmlUnit before updating it, for 2.9, removing support for JDK 7 entirely)

I'll dig this up while you work on a real fix ;-)

[Colin Alworth]

Like we do for com.google.gwt.junit.RunStyleHtmlUnit.HostedJavaScriptEngine so we can hook in the "plugin". Looks like that idea might be a winner! Just make sure to swap it in both cases, don't want to kill tests in old dev mode.

[Thomas Broyer]

On Saturday, October 7, 2017 at 4:54:28 PM UTC+2, Colin Alworth wrote:

Like we do for com.google.gwt.junit.RunStyleHtmlUnit.HostedJavaScriptEngine so we can hook in the "plugin". Looks like that idea might be a winner! Just make sure to swap it in both cases, don't want to kill tests in old dev mode.

Seems to be working. I still have 2 tests failing in JUnitSuite (GWTTestCaseSetupTearDownTest#testSetUpTearDownTimeout and GWTTestCaseAsyncTest#testLateFinsh_assert), both of them expecting TimeoutException and getting a SerializableThrowable instead. Need to debug this using -Dgwt.htmlunit.debug now (exception thrown from Impl.java:215, i.e. Impl#reportToBrowser, the object is logged as "[object Object]" so I need a debugger to understand what happens)

[Thomas Broyer]

OK, so, there's a problem of isolation: the exception from testSetUpTearDownFailAsync is making testSetUpTearDownTimeout fail.

This is likely because GWTTestCase's reportUncaughtException (called from the Impl.uncaughtExceptionHandlerForTest) immediately reportResultsAndRunNextMethod, which schedules the next test with Scheduler.get().scheduleDeferred. Then (from Impl's reportUncaughtException), reportToBrowser is called which throws the exception again in a setTimeout(…, 0). scheduleDeferred uses setTimeout(…, 1), but because HtmlUnit clamps the delay at 1ms anyway, the scheduleDeferred command is run before the exception is thrown to the browser.

Removing the setTimeout from Impl's reportToBrowser fixes the test, but (probably) only because reportUncaughtException is called by the top-most $entry(); otherwise it'd (probably) be caught by the parent $entry and re-routed to the UncaughtExceptionHandler.

Daniel, is there any other change you have internally that's not sync'd to open source? I don't quite understand how that could work even with the earlier HtmlUnit.

[Thomas Broyer]

Fwiw, I reverted to HtmlUnit 2.13 (and Jetty 8) and the two same tests fail, with the same error. So if it passes inside Google, then there must be something else to do, that you probably have done inside Google and not replicated to open source. 

[Colin Alworth]

Patch is accepted and merged into upstream HtmlUnit, see https://sourceforge.net/p/htmlunit/bugs/1924/ for more detail.

Daniel, when you can take a look at Thomas's question, we can get this change made to open source GWT as you requested.

[Thomas Broyer]

On Tuesday, October 10, 2017 at 6:08:05 PM UTC+2, Colin Alworth wrote:

Patch is accepted and merged into upstream HtmlUnit, see https://sourceforge.net/p/htmlunit/bugs/1924/ for more detail.

Daniel, when you can take a look at Thomas's question, we can get this change made to open source GWT as you requested.

Here's the workaround for HtmlUnit https://gwt-review.googlesource.com/c/gwt/+/19680

The only problem left is https://github.com/gwtproject/gwt/blob/a3df04c0f5469bdcf49bdd564467255951ef6559/user/super/com/google/gwt/junit/translatable/com/google/gwt/junit/client/impl/GWTRunner.java#L209-L215 which doesn't play nicely with https://github.com/gwtproject/gwt/blob/a3df04c0f5469bdcf49bdd564467255951ef6559/user/src/com/google/gwt/core/client/impl/Impl.java#L273-L277 

[Daniel Kurka]

I am not aware of anything specific we have done within Google, we never imported the jetty upgrade. I think you just go ahead for open source and make it work there. We do not care that much for htmlunit anymore within Google.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-co...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/245093f8-c1c1-4e71-9291-0b9e3af66eb9%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Thomas Broyer]

I'll try with the manual runstyle, i.e. with a real browser, and see how it goes. Thanks for the feedback.

[Goktug Gokdogan]

tbroyer: Are you using batch mode while testing? We are using -batch module internally and maybe you guys do not externally? (though you linked the code that only runs in batch mode, IIRC).

On Tue, Oct 10, 2017 at 1:04 PM, Thomas Broyer <t.br...@gmail.com> wrote:

I'll try with the manual runstyle, i.e. with a real browser, and see how it goes. Thanks for the feedback.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.

To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscribe@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/1fab1f52-557b-4857-a0a9-f017c1d4822d%40googlegroups.com.

[Thomas Broyer]

On Wednesday, October 11, 2017 at 4:52:06 AM UTC+2, Goktug Gokdogan wrote:

tbroyer: Are you using batch mode while testing? We are using -batch module internally and maybe you guys do not externally? (though you linked the code that only runs in batch mode, IIRC).

That's right, we do not use batch mode, **and** it fixes the tests! \o/

I wonder, should the default batching strategy be changed to module? or should we use "-batch module" for all tests? only HtmlUnit tests? (we only run those on CI anyway; also I tested in Firefox with -runStyle ExternalBrowser:firefox, and the tests pass without the need for -batch module)

What would you recommend?

Colin: do we try to slip this into 2.8.2 at the last minute? (and possibly revert if there's any red flag during smoke testing)

Daniel/Goktug: iiuc, this change means that you no longer need to wrap everything into $entry() to get the UncaughtExceptionHandler called on errors, right? This makes things easier with JsInterop and @JsFunction callbacks (or exposing objects whose @JsMethods will be called from JS), but as a non-negligible side effect you won't get the Scheduler.scheduleEntry and Scheduler.scheduleFinally commands called. Am I getting things right?

[Thomas Broyer]

As discussed minutes ago in meeting: here's the patch to enable -batch module for all our HtmlUnit tests: https://gwt-review.googlesource.com/c/gwt/+/19740

Once that one and the HtmlUnit workaround are in, we can rebase Daniel's patch about trapping window.onerror by default.

[Goktug Gokdogan]

There should be something around syncToServer + HtmlUnit causes the issue (I guess RPC gets called back sooner than timeout(0) but didn't debug) but I don't think this in practice will effect anything else.

Enabling batch mode by default for everything potentially effects timeouts also sometimes it might be incompatible with the test runner that drives the java side of the tests (for example it doesn't work well with our internal test sharding mechanism). Hence I don't think it worths the trouble. I think just having batch mode enabled for junit tests is good enough to move forward.

Daniel's patch is important for both hand written jsinterop code and Elemental2 and yes unfortunately Scheduler.scheduleFinally will be affected.

PS: pls avoid the urge to discuss technical stuff in steering commitee and try to keep it in the contributor list ;)

On Wed, Oct 11, 2017 at 9:59 AM, Thomas Broyer <t.br...@gmail.com> wrote:

As discussed minutes ago in meeting: here's the patch to enable -batch module for all our HtmlUnit tests: https://gwt-review.googlesource.com/c/gwt/+/19740

Once that one and the HtmlUnit workaround are in, we can rebase Daniel's patch about trapping window.onerror by default.

On Wednesday, October 11, 2017 at 10:52:41 AM UTC+2, Thomas Broyer wrote:

On Wednesday, October 11, 2017 at 4:52:06 AM UTC+2, Goktug Gokdogan wrote:

tbroyer: Are you using batch mode while testing? We are using -batch module internally and maybe you guys do not externally? (though you linked the code that only runs in batch mode, IIRC).

That's right, we do not use batch mode, **and** it fixes the tests! \o/

I wonder, should the default batching strategy be changed to module? or should we use "-batch module" for all tests? only HtmlUnit tests? (we only run those on CI anyway; also I tested in Firefox with -runStyle ExternalBrowser:firefox, and the tests pass without the need for -batch module)

What would you recommend?

Colin: do we try to slip this into 2.8.2 at the last minute? (and possibly revert if there's any red flag during smoke testing)

Daniel/Goktug: iiuc, this change means that you no longer need to wrap everything into $entry() to get the UncaughtExceptionHandler called on errors, right? This makes things easier with JsInterop and @JsFunction callbacks (or exposing objects whose @JsMethods will be called from JS), but as a non-negligible side effect you won't get the Scheduler.scheduleEntry and Scheduler.scheduleFinally commands called. Am I getting things right?

 

On Tue, Oct 10, 2017 at 1:04 PM, Thomas Broyer <t.br...@gmail.com> wrote:

I'll try with the manual runstyle, i.e. with a real browser, and see how it goes. Thanks for the feedback.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.

To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsu...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/1fab1f52-557b-4857-a0a9-f017c1d4822d%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/f9fcbd96-ae33-430e-8fda-1cca8ad8b07a%40googlegroups.com.

[Colin Alworth]

On Wed, Oct 11, 2017, at 02:01 PM, 'Goktug Gokdogan' via GWT Contributors wrote:

PS: pls avoid the urge to discuss technical stuff in steering commitee and try to keep it in the contributor list ;)

If we could get the monthly (weekly?) contrib calls going again so we have someone to pick their brains, that would help a lot. Emails work, but a lot of this is far faster to solve and pass around in bursty low-latency discussion.

Make them 15 minutes, officially, and of folks have time to stick around longer, great?

Alternatively, gitter.im/gwtproject/gwt is extremely active, and only missing representation from google. LIkewise could schedule some time to quickly discuss things.

Otherwise it all ends up in private IMs and hangouts, opaque to the rest of the world. Of course, a cynic might suggest that this has been the M.O. of gwt development even since the tool was open sourced... Do notice how light on details the original email in this thread was (and incorrect, too, based on the work Thomas and I have done since).

[Jens]

Alternatively, gitter.im/gwtproject/gwt is extremely active, and only missing representation from google. LIkewise could schedule some time to quickly discuss things.

There is also https://gitter.im/gwtproject itself which can only be joined by members of the Github GWT organization. So this room could be used as a gwt-contrib chat.

-- J.

[Goktug Gokdogan]

I think waiting for a month to discuss issues in person is impractical when you can use the contributor list and usually get responses less than a day. Plus the discussion is documented automatically for future reference.

For the cases of releases where more issues / discussions needed (not always); I agree that we could setup a time to meet to process things quicker in bulk (gitter SGTM) .

--
You received this message because you are subscribed to the Google Groups "GWT Contributors" group.
To unsubscribe from this group and stop receiving emails from it, send an email to google-web-toolkit-contributors+unsubscribe@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/google-web-toolkit-contributors/dcb140db-cb06-4cca-8d29-51364f5a95a9%40googlegroups.com.

[Thomas Broyer]

On Wednesday, October 11, 2017 at 9:02:26 PM UTC+2, Goktug Gokdogan wrote:

There should be something around syncToServer + HtmlUnit causes the issue (I guess RPC gets called back sooner than timeout(0) but didn't debug) but I don't think this in practice will effect anything else.

Enabling batch mode by default for everything potentially effects timeouts also sometimes it might be incompatible with the test runner that drives the java side of the tests (for example it doesn't work well with our internal test sharding mechanism). Hence I don't think it worths the trouble. I think just having batch mode enabled for junit tests is good enough to move forward.

(Assuming you mean the c.g.g.junit tests here) we only have one big target (with many variants) in user/build.xml that runs all the tests. I added "-batch module" to all the HtmlUnit (non-devmode) variants.

 

Daniel's patch is important for both hand written jsinterop code and Elemental2 and yes unfortunately Scheduler.scheduleFinally will be affected.

Thanks for the confirmation, and above details.

 

PS: pls avoid the urge to discuss technical stuff in steering commitee and try to keep it in the contributor list ;)

+1

It just happens that we were having this meeting and were discussing 2.8.2. Daniel confirmed that it should only affect HtmlUnit, and pushed for its inclusion in 2.8.2. We thus decided to give it a try. But I still did expect written answers in this thread :)

Also, I sent the patch just before shutting down my PC for the evening and getting home (hey, UTC+2 here ;-) ), hoping that they could get merged over night and have the tests exercised; except, well, I forgot that the HtmlUnit workaround patch had checkstyle issues…