search

eclipse-ee4j / mojarra

Mojarra, a Jakarta Faces implementation
Other
158 stars 107 forks source link

Premature invocation of h:outputLink on Firefox #5414

Closed liefke closed 3 months ago

liefke commented 3 months ago

Describe the bug

When a browser and server with HTTP/2 Push support is used (like FireFox and WildFly), all URLs referenced by an <h:outputLink value="..."> are pushed to the client, which leads to premature requests for these resources.

To Reproduce

Steps to reproduce the behavior:

  1. Create a webapp with this index.xhtml:

    <!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<f:view xmlns:f="http://java.sun.com/jsf/core" xmlns:h="http://java.sun.com/jsf/html">
<html xmlns="http://www.w3.org/1999/xhtml">
    <h:head /> 
    <h:body>
        <h:outputLink value="/logout">Logout</h:outputLink><br/>
        <h:outputLink value="https://www.google.de/" target="_BLANK">Google</h:outputLink><br/>
        <h:outputLink value="/">Home</h:outputLink><br/>
    </h:body>
</html>
</f:view>

  2. Turn on the access log of your server (I used WildFly 31.0.1.Final for testing).

  3. Start the server and deploy the webapp

  4. Open your webapp in the current Firefox 123.0.1 using HTTPS (to trigger usage of HTTP/2)

  5. Check the access logs.

  6. You will find the following:

    "GET / HTTP/2.0" 200 252
"GET /logout HTTP/1.1" 404 68
"GET /https://www.google.de/ HTTP/1.1" 404 68
"GET / HTTP/1.1" 200 252

As you can see, each outputLink is "prefetched" by Firefox, even the external URL.

In combination with some redirects in my application I even managed (unintentionally) to produce an endless loop of requests.

Expected behavior

I only see one request for "/" in the access log, nothing else.

Versions

Workarounds

Turn off HTTP/2 Push in your server, example for the standalone.xml in WildFly:

<https-listener ... http2-enable-push="false" />

Or create an ExternalContext wrapper in your application and overwrite encodeResourceURL:

public String encodeResourceURL(String url) {
    return ((HttpServletResponse) getResponse()).encodeURL(url);
}

Analysis

I would never expect a HTTP/2 Push for URLs of a h:commandLink. If necessary at all, JSF should only push resources of h:graphicImage and should not use ExternalContextImpl.encodeResourceUrl for that purpose, as the push is an unexpected side effect of a call to that method. In addition even h:graphicImage should check, if the URL is an external URL.

Nevertheless I would remove HTTP/2 Push support from JSF at all, see the quote here:

In practice, Server Push frequently results in wasted bandwidth because the server rarely knows which resources are already loaded by the client and transmits the same resource multiple times, resulting in slowdowns if the resources being pushed compete for bandwidth with resources that were requested.

liefke commented 3 months ago

By the way, I'd guess that the current Push implementation is even the root cause for #5391, at least on Firefox.

BalusC commented 3 months ago

This is part of the spec: https://jakarta.ee/specifications/faces/4.0/jakarta-faces-4.0#a457

If running on a container that supports Jakarta Servlet 4.0 or later, after any dynamic component manipulations have been completed, any resources that have been added to the UIViewRoot, such as scripts, images, or stylesheets, and any inline images, must be pushed to the client using the Jakarta Servlet Server Push API. All of the pushes must be started before any of the HTML of the response is rendered to the client.

So removing is not an option. However, you're right that it should not have pushed commandlinks let alone external resources. The implementation is indeed incorrect in this regard.

liefke commented 2 months ago

Thanks for fixing this.

But the reference to the spec is only half the truth - as you could deal with the issue in the next release of the spec. I guess that you understand the argument of the wasted bandwidth and that HTTP/2 Push is a dead end.

Nevertheless I still think, that encodeResourceURL is the wrong place for initiating the push, as the method could be called for any resource, not only for resources that have been added to the UIViewRoot (as required by the spec).

BalusC commented 2 months ago

Fair point.