Android range requests

[steinjak]

This PR addresses issue #2978, which seems to be a problem carried over from the cordova plugin: ionic-team/cordova-plugin-ionic-webview/issues/453

The issue is with PathHandler, where the responseHeaders map is shared between threads, causing simultaneous requests to return the same Content-Length to the browser in case of range requests. See the illustration in ionic-team/cordova-plugin-ionic-webview/issues/453 (the same issue is present Capacitor).

This PR proposes to return a copy of the map on each invocation.

Furhermore, it introduces skipping of the initial part of the file in case of a range request not starting from zero. This brings the Android implementation better in line with its iOS counterpart, see

capacitor/ios/Capacitor/Capacitor/WebViewAssetHandler.swift

Line 51 in 3d4433b

try fileHandle.seek(toOffset: UInt64(fromRange))

[peitschie]

Am just driving by while hunting out my own problem... I wonder if a better way to do this is to make the copy only when it's about to be modified here.

I.e., instead of creating a copy every access, do it when the tempResponseHeaders variable is assigned at line 208:

Map<String, String> tempResponseHeaders = new HashMap(handler.getResponseHeaders());

Other than that though, this code definitely makes good sense to me.

[steinjak]

I see what you mean (a getter should not create new objects), but this will re-introduce the problem, as there are other invocations of this method as well - all passing the returned instance into the constructor of WebResourceResponse (which is where the value of tempResponseHeaders also ends up). Note that it's not the headers set on lines 225/226 that cause trouble here, but the Content-Length header, set somewhere else downstream from this code.

We could of course move the cloning to all call points of the getter, but that leaves a trap for the next person coming around and wanting to use this method, probably not knowing about this issue, so we should probably not do it without making the map read-only (through Collections.unmodifiableMap(...), ie. this.responseHeaders = Collections.unmodifiableMap(new LinkedHashMap<String, String>(tempResponseHeaders)) on line 102). The downside to this approach, though, is that it triggers runtime exceptions - and I can't find the automated tests that will trigger these when developing, leaving it to the developer to thoroughly test all cases before submitting a patch...

Maybe it would be better to rename WebViewLocalServer.PathHandler.getResponseHeaders() to WebViewLocalServer.PathHandler.buildDefaultResponseHeaders() to clarify? I don't think I can see a case where the set of response headers need to be shared between responses, only cases where there is a base set of headers to be included in every response.

What do you think, @peitschie ?

[visuallization]

I am currently experiencing a similar issue: #6021 (comment)

Are there any updates on this PR?

[visuallization]

Furhermore, it introduces skipping of the initial part of the file in case of a range request not starting from zero

@steinjak This does sound interesting, maybe it is even enough to do this? I am just wondering if it would break anything else? I would imagine for streaming you would still want to have the option to get a different start byte, but maybe it does not matter in this part of capacitor.

[steinjak]

@visuallization I think the skipping part is unrelated here, as it's the Content-Length header that ends up with the wrong value due to a race condition. If I remember correctly, capacitor tries to serve the whole file at once, so I suspect the skipping part is mostly redundant (a long time since I looked at the code).

Sadly, there doesn't seem to be much interest in this PR.

For the next person facing this: We circumvent the issue by downloading the media file without streaming (using Angular HTTP service, fetch() would probably also work) and map it to a resource URL (URL.createObjectURL(blob)) that we point our media elements to.

[JaHollTV]

are there updates?

[visuallization]

@visuallization I think the skipping part is unrelated here, as it's the Content-Length header that ends up with the wrong value due to a race condition. If I remember correctly, capacitor tries to serve the whole file at once, so I suspect the skipping part is mostly redundant (a long time since I looked at the code).

Sadly, there doesn't seem to be much interest in this PR.

For the next person facing this: We circumvent the issue by downloading the media file without streaming (using Angular HTTP service, fetch() would probably also work) and map it to a resource URL (URL.createObjectURL(blob)) that we point our media elements to.

@steinjak I patched capacitor in our project with your fix and it turns out the skipping part is actually an issue as some audio will just stop un mid sentence. So I removed the skipping part and now everything works as intended! Thanks for the fix, it helped a lot!

[biguphpc]

Having the same issue with Range requests. This bug is easy to reproduce by putting multiple <audio> tags on a page. The concurrent request the browser makes leads to the threading issues and the range headers response corruption. Right now i'm, having to patch the Java file during the build to disable the Range requests completely which is a terrible solution.

I've tested the PR and it solves our issue. It's also a lot better than the current status quo. Let me know if I can help do something to have the PR merged. Thanks

[WoodyWoodsta]

How can we help with this PR? This seems like a fairly critical issue when loading anything larger and in parallel (a reasonable requirement) from the filesystem in Android.