Skip to content
This repository has been archived by the owner on Feb 29, 2020. It is now read-only.

Commit

Permalink
Bug 1436615 - Lots of memory used for activity stream PNGs encoded as…
Browse files Browse the repository at this point in the history 
… data URIs

Signed-off-by: Jay Lim <jlim@mozilla.com>
  • Loading branch information
Jay Lim committed May 14, 2018
1 parent c63e3c1 commit f005b63
Show file tree
Hide file tree
Showing 7 changed files with 450 additions and 103 deletions.
136 changes: 123 additions & 13 deletions system-addon/content-src/components/Card/Card.jsx
View file
Expand Up @@ -9,6 +9,9 @@ import React from "react";
// Keep track of pending image loads to only request once
const gImageLoading = new Map();

// Keep track of blob urls to only generate once for each path in the same newtab page
export const gBlobUrls = new Map();

/**
* Card component.
* Cards are found within a Section component and contain information about a link such
Expand All @@ -24,44 +27,126 @@ export class _Card extends React.PureComponent {
this.state = {
activeCard: null,
imageLoaded: false,
showContextMenu: false
showContextMenu: false,
cardImage: null
};
this.onMenuButtonClick = this.onMenuButtonClick.bind(this);
this.onMenuUpdate = this.onMenuUpdate.bind(this);
this.onLinkClick = this.onLinkClick.bind(this);
this.removeCardImageRef = this.removeCardImageRef.bind(this);
}

/**
* Helper to conditionally load an image and update state when it loads.
*/
async maybeLoadImage() {
// No need to load if it's already loaded or no image
const {image} = this.props.link;
if (!this.state.imageLoaded && image) {
const {cardImage} = this.state;
if (!cardImage) {
return;
}

const imageUrl = cardImage.url;
if (!this.state.imageLoaded) {
// Initialize a promise to share a load across multiple card updates
if (!gImageLoading.has(image)) {
if (!gImageLoading.has(imageUrl)) {
const loaderPromise = new Promise((resolve, reject) => {
const loader = new Image();
loader.addEventListener("load", resolve);
loader.addEventListener("error", reject);
loader.src = image;
loader.src = imageUrl;
});

// Save and remove the promise only while it's pending
gImageLoading.set(image, loaderPromise);
loaderPromise.catch(ex => ex).then(() => gImageLoading.delete(image)).catch();
gImageLoading.set(imageUrl, loaderPromise);
loaderPromise.catch(ex => ex).then(() => gImageLoading.delete(imageUrl)).catch();
}

// Wait for the image whether just started loading or reused promise
await gImageLoading.get(image);
await gImageLoading.get(imageUrl);

// Only update state if we're still waiting to load the original image
if (this.props.link.image === image && !this.state.imageLoaded) {
if (this.isImageInState(this.props.link.image) && !this.state.imageLoaded) {
this.setState({imageLoaded: true});
}
}
}

removeCardImageRef(event) {
if (this.state.cardImage) {
const imgMetadata = gBlobUrls.get(this.state.cardImage.path);

// Only updates refCount if card image is a blob.
if (imgMetadata) {
imgMetadata.refCount -= 1;
}
}
}

/**
* Helper to update the current state with the image obtained in props.
*/
updateCardImage(props) {
const {image} = props.link;
if (!image) {
this.removeCardImageRef();
this.setState({cardImage: null});
return;
}

if (image.type !== "blob") {
this.removeCardImageRef();
this.setState({cardImage: {url: image}});
return;
}

let changed = false;
if (this.state.cardImage && this.state.cardImage.path !== image.path) {
changed = true;
}

if (gBlobUrls.has(image.path)) {
if (changed) {
this.removeCardImageRef();
}
const imgMetadata = gBlobUrls.get(image.path);
if (!this.state.cardImage || changed) {
imgMetadata.refCount += 1;
}
this.setState({cardImage: {url: imgMetadata.url, path: image.path}});
return;
}

const objectURL = URL.createObjectURL(image.data);

gBlobUrls.set(image.path, {url: objectURL, refCount: 1});

this.setState({cardImage: {url: objectURL, path: image.path}});
}

/**
* Helper to check if an image is already in the component's state.
*/
isImageInState(image) {
const {cardImage} = this.state;

if (!image && !cardImage) {
return true;
}

if (image && cardImage) {
if (image.type === "blob") {
// cardImage and new image are both blobs.
return cardImage.path !== undefined && image.path === cardImage.path;
}

// cardImage and new image are both non-blobs.
return cardImage.path === undefined && image === cardImage.url;
}

return false;
}

onMenuButtonClick(event) {
event.preventDefault();
this.setState({
Expand Down Expand Up @@ -124,18 +209,35 @@ export class _Card extends React.PureComponent {
}

componentDidMount() {
this.props.window.addEventListener("beforeunload", this.removeCardImageRef);

this.maybeLoadImage();
}

componentDidUpdate() {
this.maybeLoadImage();
}

componentWillMount() {
this.updateCardImage(this.props);
}

componentWillReceiveProps(nextProps) {
// Clear the image state if changing images
if (nextProps.link.image !== this.props.link.image) {
if (nextProps.link && !this.isImageInState(nextProps.link.image)) {
this.setState({imageLoaded: false});
}

// NB: updateCardImage needs to be called after isImageInState since cardImage's
// state is modified in updateCardImage and isImageInState uses the old state.
this.updateCardImage(nextProps);
}

componentWillUnmount() {
this.removeCardImageRef();
this.setState({cardImage: null});

this.props.window.removeEventListener("beforeunload", this.removeCardImageRef);
}

render() {
Expand All @@ -144,8 +246,8 @@ export class _Card extends React.PureComponent {
const isContextMenuOpen = this.state.showContextMenu && this.state.activeCard === index;
// Display "now" as "trending" until we have new strings #3402
const {icon, intlID} = cardContextTypes[link.type === "now" ? "trending" : link.type] || {};
const hasImage = link.image || link.hasImage;
const imageStyle = {backgroundImage: link.image ? `url(${link.image})` : "none"};
const hasImage = this.state.cardImage || link.hasImage;
const imageStyle = {backgroundImage: this.state.cardImage ? `url(${this.state.cardImage.url})` : "none"};

return (<li className={`card-outer${isContextMenuOpen ? " active" : ""}${props.placeholder ? " placeholder" : ""}`}>
<a href={link.type === "pocket" ? link.open_url : link.url} onClick={!props.placeholder ? this.onLinkClick : undefined}>
Expand Down Expand Up @@ -194,6 +296,14 @@ export class _Card extends React.PureComponent {
</li>);
}
}
_Card.defaultProps = {link: {}};

_Card.defaultProps = {
link: {},
window: global.window || {
addEventListener: () => {},
removeEventListener: () => {}
}
};

export const Card = connect(state => ({platform: state.Prefs.values.platform}))(_Card);
export const PlaceholderCard = () => <Card placeholder={true} />;
38 changes: 37 additions & 1 deletion system-addon/content-src/components/Sections/Sections.jsx
View file
@@ -1,4 +1,4 @@
import {Card, PlaceholderCard} from "content-src/components/Card/Card";
import {Card, gBlobUrls, PlaceholderCard} from "content-src/components/Card/Card";
import {FormattedMessage, injectIntl} from "react-intl";
import {actionCreators as ac} from "common/Actions.jsm";
import {CollapsibleSection} from "content-src/components/CollapsibleSection/CollapsibleSection";
Expand Down Expand Up @@ -190,6 +190,35 @@ Section.defaultProps = {
export const SectionIntl = connect(state => ({Prefs: state.Prefs}))(injectIntl(Section));

export class _Sections extends React.PureComponent {
componentDidMount() {
this.props.window.addEventListener("beforeunload", this._gcGBlobUrls);
this._gcGBlobUrls();
}

componentDidUpdate() {
this._gcGBlobUrls();
}

componentWillUnmount() {
this._gcGBlobUrls();
this.props.window.removeEventListener("beforeunload", this._gcGBlobUrls);
}

/**
* Helper to revoke the generated object URLs for image blobs which are unused.
*/
_gcGBlobUrls() {
if (!gBlobUrls || !gBlobUrls.size) {
return;
}
for (let [path, obj] of gBlobUrls) {
if (obj.refCount === 0) {
URL.revokeObjectURL(obj.url);
gBlobUrls.delete(path);
}
}
}

renderSections() {
const sections = [];
const enabledSections = this.props.Sections.filter(section => section.enabled);
Expand Down Expand Up @@ -224,4 +253,11 @@ export class _Sections extends React.PureComponent {
}
}

_Sections.defaultProps = {
window: global.window || {
addEventListener: () => {},
removeEventListener: () => {}
}
};

export const Sections = connect(state => ({Sections: state.Sections, Prefs: state.Prefs}))(_Sections);
43 changes: 7 additions & 36 deletions system-addon/lib/Screenshots.jsm
View file
Expand Up @@ -5,18 +5,14 @@

const EXPORTED_SYMBOLS = ["Screenshots"];

Cu.importGlobalProperties(["fetch"]);

ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm");

ChromeUtils.defineModuleGetter(this, "BackgroundPageThumbs",
"resource://gre/modules/BackgroundPageThumbs.jsm");
ChromeUtils.defineModuleGetter(this, "PageThumbs",
"resource://gre/modules/PageThumbs.jsm");
ChromeUtils.defineModuleGetter(this, "FileUtils",
"resource://gre/modules/FileUtils.jsm");
XPCOMUtils.defineLazyServiceGetter(this, "MIMEService",
"@mozilla.org/mime;1", "nsIMIMEService");
ChromeUtils.defineModuleGetter(this, "OS",
"resource://gre/modules/osfile.jsm");
ChromeUtils.defineModuleGetter(this, "PrivateBrowsingUtils",
"resource://gre/modules/PrivateBrowsingUtils.jsm");
ChromeUtils.defineModuleGetter(this, "Services",
Expand All @@ -25,53 +21,28 @@ ChromeUtils.defineModuleGetter(this, "Services",
const GREY_10 = "#F9F9FA";

this.Screenshots = {
/**
* Convert bytes to a string using extremely fast String.fromCharCode without
* exceeding the max number of arguments that can be provided to a function.
*/
_bytesToString(bytes) {
// NB: This comes from js/src/vm/ArgumentsObject.h ARGS_LENGTH_MAX
const ARGS_LENGTH_MAX = 500 * 1000;
let i = 0;
let str = "";
let {length} = bytes;
while (i < length) {
const start = i;
i += ARGS_LENGTH_MAX;
str += String.fromCharCode.apply(null, bytes.slice(start, i));
}
return str;
},

/**
* Get a screenshot / thumbnail for a url. Either returns the disk cached
* image or initiates a background request for the url.
*
* @param url {string} The url to get a thumbnail
* @return {Promise} Resolves a data uri string or null if failed
* @return {Promise} Resolves a custom object or null if failed
*/
async getScreenshotForURL(url) {
try {
await BackgroundPageThumbs.captureIfMissing(url, {backgroundColor: GREY_10});
const imgPath = PageThumbs.getThumbnailPath(url);

// OS.File object used to easily read off-thread
const file = await OS.File.open(imgPath, {read: true, existing: true});
let filePathResponse = await fetch(`file://${imgPath}`);
let fileContents = await filePathResponse.blob();

// Check if the file is empty, which indicates there isn't actually a
// thumbnail, so callers can show a failure state.
const bytes = await file.read();
if (bytes.length === 0) {
if (fileContents.size === 0) {
return null;
}

// nsIFile object needed for MIMEService
const nsFile = FileUtils.File(imgPath);
const contentType = MIMEService.getTypeFromFile(nsFile);

const encodedData = btoa(this._bytesToString(bytes));
file.close();
return `data:${contentType};base64,${encodedData}`;
return {type: "blob", path: imgPath, data: fileContents};
} catch (err) {
Cu.reportError(`getScreenshot(${url}) failed: ${err}`);
}
Expand Down
12 changes: 7 additions & 5 deletions system-addon/test/functional/mochitest/browser_getScreenshots.js
View file
Expand Up @@ -11,18 +11,20 @@ const XHTMLNS = "http://www.w3.org/1999/xhtml";

ChromeUtils.defineModuleGetter(this, "Screenshots", "resource://activity-stream/lib/Screenshots.jsm");

function get_pixels_for_data_uri(dataURI, width, height) {
function get_pixels_for_blob(blob, width, height) {
return new Promise(resolve => {
// get the pixels out of the screenshot that we just took
let img = document.createElementNS(XHTMLNS, "img");
img.setAttribute("src", dataURI);
let imgPath = URL.createObjectURL(blob);
img.setAttribute("src", imgPath);
img.addEventListener("load", () => {
let canvas = document.createElementNS(XHTMLNS, "canvas");
canvas.setAttribute("width", width);
canvas.setAttribute("height", height);
let ctx = canvas.getContext("2d");
ctx.drawImage(img, 0, 0, width, height);
const result = ctx.getImageData(0, 0, width, height).data;
URL.revokeObjectURL(imgPath);
resolve(result);
}, {once: true});
});
Expand All @@ -31,9 +33,9 @@ function get_pixels_for_data_uri(dataURI, width, height) {
add_task(async function test_screenshot() {
await SpecialPowers.pushPrefEnv({set: [["browser.pagethumbnails.capturing_disabled", false]]});

// take a screenshot of a blue page and save it as a data URI
const screenshotAsDataURI = await Screenshots.getScreenshotForURL(TEST_URL);
let pixels = await get_pixels_for_data_uri(screenshotAsDataURI, 10, 10);
// take a screenshot of a blue page and save it as a blob
const screenshotAsObject = await Screenshots.getScreenshotForURL(TEST_URL);
let pixels = await get_pixels_for_blob(screenshotAsObject.data, 10, 10);
let rgbaCount = {r: 0, g: 0, b: 0, a: 0};
while (pixels.length) {
// break the pixels into arrays of 4 components [red, green, blue, alpha]
Expand Down

0 comments on commit f005b63

Please sign in to comment.