1633197 - Crashes on mozilla::dom::WebSocketImpl::OnAcknowledge

Status: RESOLVED FIXED

(Core :: Networking: WebSockets, defect, P2)

Description

[vlader101]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/81.0.4044.122 Safari/537.36

Steps to reproduce:

Crash Report
https://crash-stats.mozilla.org/report/index/a6c6ecb2-ca4a-4665-a0c6-5efcb0200425

• Create a web worker script.
• Pass the files property from a file input to the web worker/
• Take the first file element.
• Buffer the file and send it through websockets using the code below:

let size = file.size;
let blockSize = BLOCK_SIZE;
let start = 0;

while (size != 0)
{
if (size < blockSize)
blockSize = size;

    let slice = file.slice(start, start + blockSize);
    webSocket.send(slice);
        
    size -= blockSize;
    start += blockSize;

}

Actual results:

Firefox crashed rather than finishing it's task where as Chrome. and Safari don't crash.

Expected results:

Not crash and complete its task.

Comment 1

[vlader101]

const BLOCK_SIZE = 65536;

Comment 2

[Jeronimo Torti (Please send NI request to Brindusa Tot)]

Hi,
I don't have the technical knowledge to confirm this issue but I'll add product and component so the team can make some research on it. Hopefully, someone with a more deep understanding of this matter can help. Feel free to route this ticket to the corresponding team.

Regards,
Jerónimo.

Comment 3

[vlader101]

I must add, this crash occurs if the file is larger or equal to 5gb.

Comment 4

[vlader101]

I can also provide a demo link if needed.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

(In reply to vlader101 from comment #3)

I must add, this crash occurs if the file is larger or equal to 5gb.

This fails here

MOZ_RELEASE_ASSERT(mWebSocket->mOutgoingBufferedAmount.isValid());

mOutgoingBufferedAmount is a CheckedUint32 - so most likely it just overflows - 5GB doesn't fit in 32bit 🙂
I see us either using a CheckedUint64 or simply returning an error code if it's not valid.
Which one do you think is better here?

Comment 6

[vlader101]

Is mOutgoingBufferedAmount accessible from JS?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: Bug 1633197 - WebSocket bufferedAmount must be unsigned long long (uint64_t), r?valentin

Comment 8

[Andrea Marchesini [:baku]]

The question is: why the value is invalid at the point? We must keep it valid all the time.
And yes, mOutgoingBufferedAmount is accessible via JS in read-only mode.

The bug here is that sending the big blob makes mOutgoingBuferredAmount invalid. At the next check, we crash.

Comment 9

[vlader101]

Just to confirm, I added the code below in the while loop and it does prevent Firefox from crashing. (I choose a conservative number).

while (webSocket.bufferedAmount > 3000000000) {}

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d6f05f371456
WebSocket bufferedAmount must be unsigned long long (uint64_t), r=valentin

Comment 11

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/d6f05f371456