MachinePublishers / jBrowserDriver

A programmable, embeddable web browser driver compatible with the Selenium WebDriver spec -- headless, WebKit-based, pure Java
Other
809 stars 143 forks source link

The finally block in JBrowserDriverServer#get has no timeout causing pageTimeout/ajaxTimeout to not be respected. #292

Open nddipiazza opened 6 years ago

nddipiazza commented 6 years ago

Let's say I use JBrowserDriver to get an HTML document that will run forever because it has a <script>while(true) {}</script> inside.

And I set all the page,ajax,script timeouts to 2000ms.

Yet when you run the jBrowserDriver to get this page, the timeout is usually a little more than 10000ms.

It comes down to this code: https://github.com/MachinePublishers/jBrowserDriver/blob/master/src/com/machinepublishers/jbrowserdriver/JBrowserDriverServer.java#L330

    AppThread.exec(
        new Sync<Object>() {
          @Override
          public Object perform() {
            context.get().item().engine.get().getLoadWorker().cancel();
            throw new TimeoutException(new StringBuilder()
                .append("Timeout of ")
                .append(context.get().timeouts.get().getPageLoadTimeoutMS())
                .append("ms reached.").toString());
          }
        });

Because this has no timeout, this exec can take a bunch of time to wait for a status.

When I add a timeout of 1ms to this method, that makes it honor the timeout correctly. But that would probably have side effects right?

Why does this timeout have to be in an AppThread with Sync on the Status?

nddipiazza commented 6 years ago

anyone have any thoughts on this?

hollingsworthd commented 6 years ago

Just to explain the code in question... That block of code is called after a timeout has already occurred. It cancels the page load and throws an exception. The page load needs to be canceled in the JavaFX event thread which is why AppThread.exec(..) is used.

It seems that page load cancellation must be getting delayed in this case. It wasn't planned for that a cancellation could get hung up. Pegging the CPU might definitely cause that. Thanks for bringing this up! Yes that block needs a timeout or maybe there's some better way to handle all of this.