Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix media upload http 413 handling #8674

Merged
merged 1 commit into from
May 24, 2022
Merged

Fix media upload http 413 handling #8674

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 29 additions & 18 deletions src/ContentMessages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import encrypt from "matrix-encrypt-attachment";
import extractPngChunks from "png-chunks-extract";
import { IAbortablePromise, IImageInfo } from "matrix-js-sdk/src/@types/partials";
import { logger } from "matrix-js-sdk/src/logger";
import { IEventRelation, ISendEventResponse, MatrixEvent } from "matrix-js-sdk/src/matrix";
import { IEventRelation, ISendEventResponse, MatrixError, MatrixEvent } from "matrix-js-sdk/src/matrix";
import { THREAD_RELATION_TYPE } from "matrix-js-sdk/src/models/thread";

import { IEncryptedFile, IMediaEventInfo } from "./customisations/models/IMediaEventContent";
Expand Down Expand Up @@ -64,10 +64,7 @@ interface IMediaConfig {
interface IContent {
body: string;
msgtype: string;
info: {
size: number;
mimetype?: string;
};
info: IMediaEventInfo;
file?: string;
url?: string;
}
Expand Down Expand Up @@ -129,6 +126,9 @@ const IMAGE_THUMBNAIL_MIN_REDUCTION_PERCENT = 0.1; // 10%
// We don't apply these thresholds to video thumbnails as a poster image is always useful
// and videos tend to be much larger.

// Image mime types for which to always include a thumbnail for even if it is larger than the input for wider support.
const ALWAYS_INCLUDE_THUMBNAIL = ["image/avif", "image/webp"];

/**
* Read the metadata for an image file and create and upload a thumbnail of the image.
*
Expand All @@ -137,7 +137,11 @@ const IMAGE_THUMBNAIL_MIN_REDUCTION_PERCENT = 0.1; // 10%
* @param {File} imageFile The image to read and thumbnail.
* @return {Promise} A promise that resolves with the attachment info.
*/
async function infoForImageFile(matrixClient: MatrixClient, roomId: string, imageFile: File) {
async function infoForImageFile(
matrixClient: MatrixClient,
roomId: string,
imageFile: File,
): Promise<Partial<IMediaEventInfo>> {
let thumbnailType = "image/png";
if (imageFile.type === "image/jpeg") {
thumbnailType = "image/jpeg";
Expand All @@ -149,7 +153,7 @@ async function infoForImageFile(matrixClient: MatrixClient, roomId: string, imag
const imageInfo = result.info;

// For lesser supported image types, always include the thumbnail even if it is larger
if (!["image/avif", "image/webp"].includes(imageFile.type)) {
if (!ALWAYS_INCLUDE_THUMBNAIL.includes(imageFile.type)) {
// we do all sizing checks here because we still rely on thumbnail generation for making a blurhash from.
const sizeDifference = imageFile.size - imageInfo.thumbnail_info.size;
if (
Expand Down Expand Up @@ -178,7 +182,7 @@ async function infoForImageFile(matrixClient: MatrixClient, roomId: string, imag
* @param {File} videoFile The file to load in an video element.
* @return {Promise} A promise that resolves with the video image element.
*/
function loadVideoElement(videoFile): Promise<HTMLVideoElement> {
function loadVideoElement(videoFile: File): Promise<HTMLVideoElement> {
return new Promise((resolve, reject) => {
// Load the file into an html element
const video = document.createElement("video");
Expand Down Expand Up @@ -224,7 +228,11 @@ function loadVideoElement(videoFile): Promise<HTMLVideoElement> {
* @param {File} videoFile The video to read and thumbnail.
* @return {Promise} A promise that resolves with the attachment info.
*/
function infoForVideoFile(matrixClient, roomId, videoFile) {
function infoForVideoFile(
matrixClient: MatrixClient,
roomId: string,
videoFile: File,
): Promise<Partial<IMediaEventInfo>> {
const thumbnailType = "image/jpeg";

let videoInfo: Partial<IMediaEventInfo>;
Expand Down Expand Up @@ -449,7 +457,7 @@ export default class ContentMessages {
});
}

public cancelUpload(promise: Promise<any>, matrixClient: MatrixClient): void {
public cancelUpload(promise: IAbortablePromise<any>, matrixClient: MatrixClient): void {
const upload = this.inprogress.find(item => item.promise === promise);
if (upload) {
upload.canceled = true;
Expand All @@ -466,12 +474,12 @@ export default class ContentMessages {
replyToEvent: MatrixEvent | undefined,
promBefore: Promise<any>,
) {
const content: IContent = {
const content: Omit<IContent, "info"> & { info: Partial<IMediaEventInfo> } = {
body: file.name || 'Attachment',
info: {
size: file.size,
},
msgtype: "", // set later
msgtype: MsgType.File, // set more specifically later
};

attachRelation(content, relation);
Expand All @@ -497,6 +505,7 @@ export default class ContentMessages {
Object.assign(content.info, imageInfo);
resolve();
}, (e) => {
// Failed to thumbnail, fall back to uploading an m.file
logger.error(e);
content.msgtype = MsgType.File;
resolve();
Expand All @@ -510,6 +519,8 @@ export default class ContentMessages {
Object.assign(content.info, videoInfo);
resolve();
}, (e) => {
// Failed to thumbnail, fall back to uploading an m.file
logger.error(e);
content.msgtype = MsgType.File;
resolve();
});
Expand Down Expand Up @@ -541,8 +552,8 @@ export default class ContentMessages {
dis.dispatch<UploadProgressPayload>({ action: Action.UploadProgress, upload });
}

let error;
return prom.then(function() {
let error: MatrixError;
return prom.then(() => {
if (upload.canceled) throw new UploadCanceledError();
// XXX: upload.promise must be the promise that
// is returned by uploadFile as it has an abort()
Expand All @@ -567,11 +578,11 @@ export default class ContentMessages {
});
}
return prom;
}, function(err) {
}, function(err: MatrixError) {
error = err;
if (!upload.canceled) {
let desc = _t("The file '%(fileName)s' failed to upload.", { fileName: upload.fileName });
if (err.http_status === 413) {
if (err.httpStatus === 413) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matrix-org/matrix-js-sdk#2396

desc = _t(
"The file '%(fileName)s' exceeds this homeserver's size limit for uploads",
{ fileName: upload.fileName },
Expand All @@ -593,7 +604,7 @@ export default class ContentMessages {
// 413: File was too big or upset the server in some way:
// clear the media size limit so we fetch it again next time
// we try to upload
if (error && error.http_status === 413) {
if (error?.httpStatus === 413) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matrix-org/matrix-js-sdk#2396

this.mediaConfig = null;
}
dis.dispatch<UploadErrorPayload>({ action: Action.UploadFailed, upload, error });
Expand All @@ -613,7 +624,7 @@ export default class ContentMessages {
return true;
}

private ensureMediaConfigFetched(matrixClient: MatrixClient) {
private ensureMediaConfigFetched(matrixClient: MatrixClient): Promise<void> {
if (this.mediaConfig !== null) return;

logger.log("[Media Config] Fetching");
Expand Down