Remove gBlobUrls.

tldr: createObjectURL for a card's image and revokeObjectURL if the image changes. For special cases that are not handled, we'll let the document unload and revoke normally.
mozilla · May 24, 2018 · 4144033 · 4144033
1 parent be7ea5b
commit 4144033
Show file tree

Hide file tree

Showing 6 changed files with 87 additions and 252 deletions.
diff --git a/system-addon/content-src/components/Card/Card.jsx b/system-addon/content-src/components/Card/Card.jsx
@@ -9,9 +9,6 @@ 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
@@ -33,7 +30,6 @@ export class _Card extends React.PureComponent {
     this.onMenuButtonClick = this.onMenuButtonClick.bind(this);
     this.onMenuUpdate = this.onMenuUpdate.bind(this);
     this.onLinkClick = this.onLinkClick.bind(this);
-    this.removeCardImageRef = this.removeCardImageRef.bind(this);
   }
 
   /**
@@ -72,14 +68,12 @@ export class _Card extends React.PureComponent {
     }
   }
 
-  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 conditionally revoke the card image if it is a blob.
+   */
+  maybeRevokeImageBlob() {
+    if (this.state.cardImage && this.state.cardImage.path !== undefined) {
+      this.props.url.revokeObjectURL(this.state.cardImage.url);
     }
   }
 
@@ -88,60 +82,44 @@ export class _Card extends React.PureComponent {
    */
   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}});
+    if (this.isImageInState(image)) {
       return;
     }
 
-    let changed = false;
-    if (this.state.cardImage && this.state.cardImage.path !== image.path) {
-      changed = true;
-    }
+    // Since image was updated, attempt to revoke old image blob URL, if it exists.
+    this.maybeRevokeImageBlob();
 
-    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}});
+    // No new image.
+    if (!image) {
+      this.setState({cardImage: null});
       return;
     }
 
-    const objectURL = URL.createObjectURL(image.data);
-
-    gBlobUrls.set(image.path, {url: objectURL, refCount: 1});
-
-    this.setState({cardImage: {url: objectURL, path: image.path}});
+    if (typeof image === "string") { // Normal image
+      this.setState({cardImage: {url: image}});
+    } else {
+      this.setState({cardImage: {url: this.props.url.createObjectURL(image.data), 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.
+      if (typeof image === "string") {
+        return cardImage.path === undefined && image === cardImage.url;
       }
 
-      // cardImage and new image are both non-blobs.
-      return cardImage.path === undefined && image === cardImage.url;
+      // cardImage and new image are both blobs.
+      return cardImage.path !== undefined && image.path === cardImage.path;
     }
 
     return false;
@@ -209,8 +187,6 @@ export class _Card extends React.PureComponent {
   }
 
   componentDidMount() {
-    this.props.window.addEventListener("unload", this.removeCardImageRef);
-
     this.maybeLoadImage();
   }
 
@@ -234,10 +210,8 @@ export class _Card extends React.PureComponent {
   }
 
   componentWillUnmount() {
-    this.removeCardImageRef();
+    this.maybeRevokeImageBlob();
     this.setState({cardImage: null});
-
-    this.props.window.removeEventListener("unload", this.removeCardImageRef);
   }
 
   render() {
@@ -299,9 +273,9 @@ export class _Card extends React.PureComponent {
 
 _Card.defaultProps = {
   link: {},
-  window: global.window || {
-    addEventListener: () => {},
-    removeEventListener: () => {}
+  url: global.URL || {
+    createObjectURL: () => {},
+    revokeObjectURL: () => {}
   }
 };
 

diff --git a/system-addon/content-src/components/Sections/Sections.jsx b/system-addon/content-src/components/Sections/Sections.jsx
@@ -1,4 +1,4 @@
-import {Card, gBlobUrls, PlaceholderCard} from "content-src/components/Card/Card";
+import {Card, 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";
@@ -190,35 +190,6 @@ Section.defaultProps = {
 export const SectionIntl = connect(state => ({Prefs: state.Prefs}))(injectIntl(Section));
 
 export class _Sections extends React.PureComponent {
-  componentDidMount() {
-    this.props.window.addEventListener("unload", this._gcGBlobUrls);
-    this._gcGBlobUrls();
-  }
-
-  componentDidUpdate() {
-    this._gcGBlobUrls();
-  }
-
-  componentWillUnmount() {
-    this._gcGBlobUrls();
-    this.props.window.removeEventListener("unload", 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);
@@ -253,11 +224,4 @@ 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);
diff --git a/system-addon/lib/Screenshots.jsm b/system-addon/lib/Screenshots.jsm
@@ -42,7 +42,7 @@ this.Screenshots = {
         return null;
       }
 
-      return {type: "blob", path: imgPath, data: fileContents};
+      return {path: imgPath, data: fileContents};
     } catch (err) {
       Cu.reportError(`getScreenshot(${url}) failed: ${err}`);
     }