1502168 - WPT test beacon-error.sub.window.html fails (only in Firefox), due to failing to reject too-large requests

Status: NEW

(Core :: DOM: Core & HTML, defect, P5)

Description

[Daniel Holbert [:dholbert]]

https://wpt.fyi/results/beacon/beacon-error.window.html has two subtests that fail in Firefox that depend on Firefox rejecting beacons that are too large:

"Verify calling 'navigator.sendBeacon()' with a large payload returns 'false'"
"Verify the behavior after the quota is exhausted."


Filing this bug on those failures.

(The test's expectations seem to be legitimate -- it uses 64 KiB (65536) as the maxPayloadSize, and the sendBeacon documentation does say this:

> Return Value
> The sendBeacon() method returns true if the user agent is able to successfully queue the data for transfer. Otherwise it returns false.
> NOTE
> The user agent imposes limits on the amount of data that can be
> sent via this API [...]
> If the amount of data to be queued exceeds the
> user agent limit (as defined in http-network-or-cache-fetch),
> this method returns false
https://w3c.github.io/beacon/#sendbeacon-method

And http-network-or-cache-fetch is defined here, with this requirement:
> If the sum of contentLengthValue and inflightKeepaliveBytes
> is greater than 64 kibibytes, then return a network error. 
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

Comment 1

[Daniel Holbert [:dholbert]]

Also, it looks like this test was renamed in the past ~day. It's now called "beacon-error.sub.window.html".  The live test is here: http://w3c-test.org/beacon/beacon-error.sub.window.html

(I suppose the wpt.fyi report will eventually be at https://wpt.fyi/results/beacon/beacon-error.sub.window.html as well, though that page doesn't have any info yet.)

Comment 2

[Daniel Holbert [:dholbert]]

(Also, for the record, I initially filed this as a test-bug at https://github.com/web-platform-tests/wpt/issues/13662 before I was aware that there was justification for the test's hardcoded 64 KiB limit. I've now closed that test-bug in favor of this Firefox bug.)

Comment 3

[Andrea Marchesini [:baku]]

Mayhemer, do we have anything in necko to know the size of a keep-alive session buffer?

See https://w3c.github.io/beacon/#sec-processing-model:

3.2.6.2: "If the amount of data that can be queued to be sent by keepalive enabled requests is exceeded by the size of transmittedData (as defined in http-network-or-cache-fetch), set the return value to false and terminate these steps. "

And: https://fetch.spec.whatwg.org/#http-network-or-cache-fetch

4.5.5.8.5: "If the sum of contentLengthValue and inflightKeepaliveBytes is greater than 64 kibibytes, then return a network error."

Note that this is not for a single navigator.sendBeacon(), but multiple sendBeacon() calls should be accumulated in the same keepalive section:

https://searchfox.org/mozilla-central/rev/e0c879c86b95bdc752b1dbff6088169735674e4a/testing/web-platform/tests/beacon/beacon-error.sub.window.js#31-37

Comment 5

[Honza Bambas (:mayhemer)]

Oh, I don't know the beacon impl at all..  sorry.  Can you search in blame for the person that worked or reviewed the code?

Comment 6

[Andrea Marchesini [:baku]]

Bug 936340 - Richard Barnes, r=sicking

Comment 7

[Honza Bambas (:mayhemer)]

OK, if you want me to answer the question, I will have to do it on Monday, sorry.  I really don't even know what `beacon` is and I have no idea what you mean with `keep-alive session buffer`.  Hence, some study is needed on my side first.

Comment 8

[Honza Bambas (:mayhemer)]

(didn't want to drop ni?me)

Comment 9

[Honza Bambas (:mayhemer)]

This has been discussed on IRC with :baku.  

Here is the summary:
- the 64k limit, as I understand it and how it only makes sense to me, is to prevent flooding memory with data queued for sending.  it may happen that, when upload is slow, queued data starts piling up w/o giving any back-pressure to the fetch() consumer and potentially lead to OOM.  
- if this limit should be per-origin or per something else (or a combo of the two) was not discussed
- to implement this, we need to know the scope of the limit and then figure out the right implementation.  it may all happen just in DOM/fetch, probably no need for any new code in Necko
- I think the wording `keep-alive` does not refer HTTP keep-alive, but it's not really clear what that means in the spec; this has to be cleared

(In reply to Andrea Marchesini [:baku] from comment #3)
> Mayhemer, do we have anything in necko to know the size of a keep-alive
> session buffer?

We agreed that there was no such thing as a `keep-alive session buffer` in Necko.  To answer the question in the context of what I wrote above: no, we don't.