Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue with "Native messaging": current example flushes all data after one message has been read - even if there are left-over data/messages in the buffer array. #2059

Open
aliko-str opened this issue Feb 3, 2021 · 5 comments · May be fixed by #29104
Assignees
Labels
Content:WebExt WebExtensions docs help wanted If you know something about this topic, we would love your help! p4 A minor issue on a Tier 2 MDN doc.

Comments

@aliko-str
Copy link

MDN URL: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/Native_messaging

What information was incorrect, unhelpful, or incomplete?

The nodejs example of the native app processing messages from FF.

The current example flushes all data ("chunks.splice(0);") after one message has been read - even if there are left-over data/messages in the buffer array.

Specific section or headline?

"App side"

What did you expect to see?

A quick fix would be to push leftover data in the buffer array after buffer flushing, and then process Data again:

		// Record leftover data to add back in Chunks
		let leftoverData = stringData.slice((payloadSize + 4)); // Note: flushChunksQueue() zeroes payloadSize
		
		// Reset the read size and the queued chunks
		flushChunksQueue();

		// Add leftover data back in Buffer and repeat processing
		if(leftoverData.length){
			chunks.push(leftoverData);
			processData();
		}

Did you test this? If so, how?

Yes. Run my extension and saw that the issue of disappearing/unprocessed messages has gone away.

MDN Content page report details
@Ryuno-Ki Ryuno-Ki added the Content:WebExt WebExtensions docs label Feb 3, 2021
@CreaTorAlexander
Copy link
Contributor

I can update this 👍

@Ryuno-Ki
Copy link
Collaborator

Ryuno-Ki commented Feb 8, 2021

Take it away, Alexander!

@CreaTorAlexander
Copy link
Contributor

@aliko-str would this be the right example?

#!/usr/local/bin/node

(() => {

    let payloadSize = null;

    // A queue to store the chunks as we read them from stdin.
    // This queue can be flushed when `payloadSize` data has been read
    let chunks = [];

    // Only read the size once for each payload
    const sizeHasBeenRead = () => Boolean(payloadSize);

      // Record leftover data to add back in Chunks
         let leftoverData = stringData.slice((payloadSize + 4)); // Note: flushChunksQueue() zeroes payloadSize
		
	// Reset the read size and the queued chunks
	flushChunksQueue();

	// Add leftover data back in Buffer and repeat processing
	if(leftoverData.length){
		chunks.push(leftoverData);
		processData();
	}

    const processData = () => {
        // Create one big buffer with all all the chunks
        const stringData = Buffer.concat(chunks);

        // The browser will emit the size as a header of the payload,
        // if it hasn't been read yet, do it.
        // The next time we'll need to read the payload size is when all of the data
        // of the current payload has been read (ie. data.length >= payloadSize + 4)
        if (!sizeHasBeenRead()) {
            payloadSize = stringData.readUInt32LE(0);
        }

        // If the data we have read so far is >= to the size advertised in the header,
        // it means we have all of the data sent.
        // We add 4 here because that's the size of the bytes that old the payloadSize
        if (stringData.length >= (payloadSize + 4)) {
            // Remove the header
            const contentWithoutSize = stringData.slice(4, (payloadSize + 4));

            // Reset the read size and the queued chunks
            flushChunksQueue();

            const json = JSON.parse(contentWithoutSize);
            // Do something with the data...
            }
    };

    process.stdin.on('readable', () => {
        // A temporary variable holding the nodejs.Buffer of each
        // chunk of data read off stdin
        let chunk = null;

        // Read all of the available data
        while ((chunk = process.stdin.read()) !== null) {
            chunks.push(chunk);
        }

        processData();

    });
})();
@aliko-str
Copy link
Author

I'm not sure you've attached the code that you meant...

I think I'd do smth like the code below, though it could probably be improved (MDN shouldn't show imperfection..): sizeHasBeenRead seems unnecessary (I don't think it has a practically smaller overhead than re-reading payloadSize; if anything, repeating Buffer.concat every time is worse), flushing and then putting data back in 'chunks' feels wrong, recursion (processData from processData) usually means I was too lazy to do things properly, 'chunks' can be a const, 'payloadSize' can be kept inside processData if it's re-read, flushChunksQueue is then unnecessary and can be reduced down to 'chunks.splice(0);', and maybe smth else.

#!/usr/local / bin / node

(() => {

let payloadSize = null;

// A queue to store the chunks as we read them from stdin.
// This queue can be flushed when `payloadSize` data has been read
let chunks = [];

// Only read the size once for each payload
const sizeHasBeenRead = () => Boolean(payloadSize);

// All the data has been read, reset everything for the next message
const flushChunksQueue = () => {
	payloadSize = null;
	chunks.splice(0);
};

const processData = () => {
	// Create one big buffer with all all the chunks
	const stringData = Buffer.concat(chunks);

	// The browser will emit the size as a header of the payload,
	// if it hasn't been read yet, do it.
	// The next time we'll need to read the payload size is when all of the data
	// of the current payload has been read (ie. data.length >= payloadSize + 4)
	if (!sizeHasBeenRead()) {
		payloadSize = stringData.readUInt32LE(0);
	}

	// If the data we have read so far is >= to the size advertised in the header,
	// it means we have all of the data sent.
	// We add 4 here because that's the size of the bytes that old the payloadSize
	if (stringData.length >= (payloadSize + 4)) {
		// Remove the header
		const contentWithoutSize = stringData.slice(4, (payloadSize + 4));

		//If FF sent more than 1 message, record leftover data to be added back in Chunks
		let leftoverData = stringData.slice((payloadSize + 4)); // Note: flushChunksQueue() zeroes payloadSize, which is why we save leftoverData here

		// Reset the read size and the queued chunks
		flushChunksQueue();

		const json = JSON.parse(contentWithoutSize);
		// Do something with the data...

		// Add the leftover data back in Buffer and repeat processing
		if (leftoverData.length) {
			chunks.push(leftoverData);
			processData();
		}

	}
};

process.stdin.on('readable', () => {
	// A temporary variable holding the nodejs.Buffer of each
	// chunk of data read off stdin
	let chunk = null;

	// Read all of the available data
	while ((chunk = process.stdin.read()) !== null) {
		chunks.push(chunk);
	}

	processData();

});

})();

@CreaTorAlexander
Copy link
Contributor

CreaTorAlexander commented Feb 15, 2021

@aliko-str would you like to submit a PR with you changes?

@CreaTorAlexander CreaTorAlexander removed their assignment Mar 18, 2021
@rebloor rebloor added the p4 A minor issue on a Tier 2 MDN doc. label Aug 6, 2021
@github-actions github-actions bot added the idle label Dec 8, 2021
@sideshowbarker sideshowbarker changed the title Issue with "Native messaging": (short summary here please) Feb 17, 2022
@teoli2003 teoli2003 reopened this May 29, 2022
@github-actions github-actions bot added the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 29, 2022
@sideshowbarker sideshowbarker removed the needs triage Triage needed by staff and/or partners. Automatically applied when an issue is opened. label May 30, 2022
@sideshowbarker sideshowbarker added the help wanted If you know something about this topic, we would love your help! label Jul 6, 2022
@rebloor rebloor linked a pull request Sep 14, 2023 that will close this issue
@rebloor rebloor self-assigned this Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Content:WebExt WebExtensions docs help wanted If you know something about this topic, we would love your help! p4 A minor issue on a Tier 2 MDN doc.
7 participants