Abort image prefetch if `Content-Length` exceeds limit #1567

dgw · 2017-09-24T11:47:13Z

Does adding this beg for the addition of tests? If so, someone will probably have to guide me in doing that. 😁

Closes #1563.

xPaw · 2017-09-24T11:50:13Z

src/plugins/irc-events/link.js

+			} else {
+				// response is an image
+				// if Content-Length header reports a size exceeding the prefetch limit, abort fetch
+				const contentLength = parseInt(res.headers["content-length"], 10) || NaN;


parseInt returns NaN on it's own, I would change it to 0 though, then you don't need to the AND check below.

Testing in Chrome's dev console with a simple literal object, running parseInt on a key that didn't exist set the test var to undefined. But using 0 instead is sensible. I went with NaN and the && in case a negative limit (i.e. no limit) is/will be possible.

xPaw · 2017-09-24T11:50:30Z

src/plugins/irc-events/link.js

@@ -163,6 +163,13 @@ function fetch(uri, cb) {
 				// if not image, limit download to 50kb, since we need only meta tags
 				// twitter.com sends opengraph meta tags within ~20kb of data for individual tweets
 				limit = 1024 * 50;
+			} else {


Can you invert the if above, it will look cleaner.

I'm not sure what you're asking me to do, but my interpretation is to make the first if test for whether it is an image, rather than not, and swap the code blocks.

xPaw · 2017-09-24T12:00:17Z

src/plugins/irc-events/link.js

+				// if Content-Length header reports a size exceeding the prefetch limit, abort fetch
+				const contentLength = parseInt(res.headers["content-length"], 10) || NaN;
+				if (contentLength && contentLength > limit) {
+					res.req.abort();


This can be res.abort.

OK, I was copying the code in the pipe handler below because I couldn't find a reference to abort() in the docs for request. For future reference, where is it documented that you can call abort() directly on the response passed to this event handler?

xPaw · 2017-09-24T12:00:28Z

src/plugins/irc-events/link.js

+			if ((/^image\/.+/.test(res.headers["content-type"]))) {
+				// response is an image
+				// if Content-Length header reports a size exceeding the prefetch limit, abort fetch
+				const contentLength = parseInt(res.headers["content-length"], 10) || NaN;


Do the 0 as I suggested.

This and the other comments in the batch are done.

xPaw · 2017-09-24T12:00:36Z

src/plugins/irc-events/link.js

@@ -159,7 +159,14 @@ function fetch(uri, cb) {
 	var limit = Helper.config.prefetchMaxImageSize * 1024;
 	req
 		.on("response", function(res) {
-			if (!(/^image\/.+/.test(res.headers["content-type"]))) {
+			if ((/^image\/.+/.test(res.headers["content-type"]))) {


You can remove extra parenthesis.

astorije · 2017-09-24T16:35:33Z

src/plugins/irc-events/link.js

+				// if Content-Length header reports a size exceeding the prefetch limit, abort fetch
+				const contentLength = parseInt(res.headers["content-length"], 10) || 0;
+				if (contentLength > limit) {
+					res.abort();


@xPaw, in #1554, we went for req.abort(). I wonder if both do the same thing, but should we use the same thing then? Which do you prefer?

abort calls abort on req internally, so yes.

@dgw, mind switching to req.abort() then?

There's no req parameter passed to this handler, only res.

You have access to req from L160. It's also being used at R179 :)

What. Has variable scoping changed since I learned JS (over a decade ago, I feel old 😁)? L160 is outside the function definitions in both cases; how does that work? 😕

(Also, L179 seems silly with the req.response.req thing… It's not related to this PR but now I really want to make it just req.abort()…)

It should be req.abort, yeah.

astorije · 2017-09-24T16:44:18Z

src/plugins/irc-events/link.js

+			if (/^image\/.+/.test(res.headers["content-type"])) {
+				// response is an image
+				// if Content-Length header reports a size exceeding the prefetch limit, abort fetch
+				const contentLength = parseInt(res.headers["content-length"], 10) || 0;


I know you guys had some discussion about this so sorry if I missed something, but it looks to me that you don't even need || 0 (which I find more misleading than explicit):

> const limit = 2048 undefined > parseInt({}["content-length"], 10) NaN > NaN > limit false > null > limit false > undefined > limit false > 4096 > limit true

So whatever parseInt returns, if the value is not an integer, the check below will be false.

It's not misleading as it will force an integer type.

It's not necessary, I don't think we should do it.

I don't like comparing the wrong data type if I can avoid it. Passing NaN into anything number-related feels kinda dirty…

Fair enough, I guess.

If the Content-Length header is present in the response when an image is prefetched, The Lounge can avoid wasting bandwidth (both for itself and for the image's host) if the value of the header exceeds the prefetch size limit by aborting the request immediately.

xPaw reviewed Sep 24, 2017

View reviewed changes

xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Sep 24, 2017

dgw force-pushed the preview-length-abort branch from b51f7d0 to 0554702 Compare September 24, 2017 11:59

xPaw reviewed Sep 24, 2017

View reviewed changes

dgw force-pushed the preview-length-abort branch from 0554702 to 2442d76 Compare September 24, 2017 12:02

xPaw approved these changes Sep 24, 2017

View reviewed changes

dgw force-pushed the preview-length-abort branch from 2442d76 to 59e311c Compare September 24, 2017 12:07

astorije reviewed Sep 24, 2017

View reviewed changes

astorije assigned dgw Sep 25, 2017

dgw force-pushed the preview-length-abort branch from 59e311c to 09cd9ce Compare September 25, 2017 10:31

astorije approved these changes Sep 25, 2017

View reviewed changes

xPaw added this to the 2.5.0 milestone Sep 25, 2017

xPaw merged commit 08edc43 into thelounge:master Sep 25, 2017

astorije changed the title ~~Abort image prefetch if Content-Length exceeds limit~~ Abort image prefetch if Content-Length exceeds limit Sep 29, 2017

dgw deleted the preview-length-abort branch June 2, 2018 18:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abort image prefetch if `Content-Length` exceeds limit #1567

Abort image prefetch if `Content-Length` exceeds limit #1567

dgw commented Sep 24, 2017

xPaw Sep 24, 2017

dgw Sep 24, 2017

xPaw Sep 24, 2017

dgw Sep 24, 2017

xPaw Sep 24, 2017

dgw Sep 24, 2017

xPaw Sep 24, 2017

dgw Sep 24, 2017

xPaw Sep 24, 2017

dgw Sep 24, 2017

xPaw Sep 24, 2017

astorije Sep 24, 2017

xPaw Sep 24, 2017

astorije Sep 24, 2017

dgw Sep 24, 2017

astorije Sep 25, 2017

dgw Sep 25, 2017 •

edited

Loading

xPaw Sep 25, 2017 •

edited

Loading

astorije Sep 24, 2017

xPaw Sep 24, 2017

astorije Sep 24, 2017

dgw Sep 24, 2017

astorije Sep 25, 2017

Abort image prefetch if Content-Length exceeds limit #1567

Abort image prefetch if Content-Length exceeds limit #1567

Conversation

dgw commented Sep 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgw Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

xPaw Sep 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Abort image prefetch if `Content-Length` exceeds limit #1567

Abort image prefetch if `Content-Length` exceeds limit #1567

dgw Sep 25, 2017 •

edited

Loading

xPaw Sep 25, 2017 •

edited

Loading