Bug 1096612 - Properly handle exceptions thrown while parsing article. r=bnicholson
authorMargaret Leibovic <margaret.leibovic@gmail.com>
Tue, 11 Nov 2014 17:11:30 -0800
changeset 215107 2216c4695b89291d2310481b91ecb448c60d2235
parent 215106 9c862ea1669face1d6baee094aa5ee4d0af9819f
child 215108 2b5cade470df3607d0a43aaf359b2257af096568
push id9906
push usermleibovic@mozilla.com
push dateWed, 12 Nov 2014 01:11:42 +0000
treeherderfx-team@2216c4695b89 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbnicholson
bugs1096612
milestone36.0a1
Bug 1096612 - Properly handle exceptions thrown while parsing article. r=bnicholson
mobile/android/chrome/content/Reader.js
mobile/android/chrome/content/aboutReader.js
mobile/android/chrome/content/browser.js
--- a/mobile/android/chrome/content/Reader.js
+++ b/mobile/android/chrome/content/Reader.js
@@ -97,17 +97,20 @@ let Reader = {
     let tab = BrowserApp.getTabForId(tabID);
     if (!tab) {
       throw new Error("Can't add tab to reading list because no tab found for ID: " + tabID);
     }
 
     let uri = tab.browser.currentURI;
     let urlWithoutRef = uri.specIgnoringRef;
 
-    let article = yield this.getArticle(urlWithoutRef, tabID);
+    let article = yield this.getArticle(urlWithoutRef, tabID).catch(e => {
+      Cu.reportError("Error getting article for tab: " + e);
+      return null;
+    });
     if (!article) {
       // If there was a problem getting the article, just store the
       // URL and title from the tab.
       article = { url: urlWithoutRef, title: tab.browser.contentDocument.title };
     }
 
     this.addArticleToReadingList(article);
   }),
@@ -140,17 +143,16 @@ let Reader = {
   /**
    * Gets an article for a given URL. This method will download and parse a document
    * if it does not find the article in the tab data or the cache.
    *
    * @param url The article URL.
    * @param tabId (optional) The id of the tab where we can look for a saved article.
    * @return {Promise}
    * @resolves JS object representing the article, or null if no article is found.
-   * @rejects Never.
    */
   getArticle: Task.async(function* (url, tabId) {
     // First, look for an article object stored on the tab.
     let tab = BrowserApp.getTabForId(tabId);
     if (tab) {
       let article = tab.savedArticle;
       if (article && article.url == url) {
         this.log("Saved article found in tab");
@@ -163,20 +165,17 @@ let Reader = {
     let article = yield this.getArticleFromCache(uri);
     if (article) {
       this.log("Saved article found in cache");
       return article;
     }
 
     // Article hasn't been found in the cache, we need to
     // download the page and parse the article out of it.
-    return yield this._downloadAndParseDocument(url).catch(e => {
-      Cu.reportError("Error downloading and parsing article: " + e);
-      return null;
-    });
+    return yield this._downloadAndParseDocument(url);
   }),
 
   /**
    * Gets an article from a loaded tab's document. This method will parse the document
    * if it does not find the article in the tab data or the cache.
    *
    * @param tab The loaded tab.
    * @return {Promise}
@@ -264,40 +263,42 @@ let Reader = {
 
     return true;
   },
 
   _readerParse: function (uri, doc) {
     return new Promise((resolve, reject) => {
       let numTags = doc.getElementsByTagName("*").length;
       if (numTags > this.MAX_ELEMS_TO_PARSE) {
-        reject("Aborting parse for " + uri.spec + "; " + numTags + " elements found");
+        this.log("Aborting parse for " + uri.spec + "; " + numTags + " elements found");
+        resolve(null);
         return;
       }
 
       let worker = new ChromeWorker("readerWorker.js");
-      worker.onmessage = function (evt) {
+      worker.onmessage = evt => {
         let article = evt.data;
 
         if (!article) {
-          reject("Worker did not return an article");
+          this.log("Worker did not return an article");
+          resolve(null);
           return;
         }
 
         // Append URL to the article data. specIgnoringRef will ignore any hash
         // in the URL.
         article.url = uri.specIgnoringRef;
         let flags = Ci.nsIDocumentEncoder.OutputSelectionOnly | Ci.nsIDocumentEncoder.OutputAbsoluteLinks;
         article.title = Cc["@mozilla.org/parserutils;1"].getService(Ci.nsIParserUtils)
                                                         .convertToPlainText(article.title, flags, 0);
         resolve(article);
       };
 
-      worker.onerror = function (evt) {
-        reject(evt.message);
+      worker.onerror = evt => {
+        reject("Error in worker: " + evt.message);
       };
 
       try {
         worker.postMessage({
           uri: {
             spec: uri.spec,
             host: uri.host,
             prePath: uri.prePath,
--- a/mobile/android/chrome/content/aboutReader.js
+++ b/mobile/android/chrome/content/aboutReader.js
@@ -502,17 +502,20 @@ AboutReader.prototype = {
       type: "SystemUI:Visibility",
       visible: visible
     });
   },
 
   _loadArticle: Task.async(function* (url, tabId) {
     this._showProgressDelayed();
 
-    let article = yield gChromeWin.Reader.getArticle(url, tabId);
+    let article = yield gChromeWin.Reader.getArticle(url, tabId).catch(e => {
+      Cu.reportError("Error loading article: " + e);
+      return null;
+    });
     if (article) {
       this._showContent(article);
     } else {
       this._win.location.href = url;
     }
   }),
 
   _requestFavicon: function Reader_requestFavicon() {
--- a/mobile/android/chrome/content/browser.js
+++ b/mobile/android/chrome/content/browser.js
@@ -4210,53 +4210,61 @@ Tab.prototype = {
                 response: this.response
               });
             }
           };
           xhr.send(this.tilesData);
           this.tilesData = null;
         }
 
-        if (!Reader.isEnabledForParseOnLoad)
+        if (!Reader.isEnabledForParseOnLoad) {
           return;
+        }
+
+        let resetReaderFlags = currentURL => {
+          // Don't clear the article for about:reader pages since we want to
+          // use the article from the previous page.
+          if (!currentURL.startsWith("about:reader")) {
+            this.savedArticle = null;
+            this.readerEnabled = false;
+            this.readerActive = false;
+          } else {
+            this.readerActive = true;
+          }
+        };
 
         // Once document is fully loaded, parse it
         Reader.parseDocumentFromTab(this).then(article => {
           // The loaded page may have changed while we were parsing the document. 
           // Make sure we've got the current one.
-          let uri = this.browser.currentURI;
-          let tabURL = uri.specIgnoringRef;
-          // Do nothing if there's no article or the page in this tab has
-          // changed
-          if (article == null || (article.url != tabURL)) {
-            // Don't clear the article for about:reader pages since we want to
-            // use the article from the previous page
-            if (!tabURL.startsWith("about:reader")) {
-              this.savedArticle = null;
-              this.readerEnabled = false;
-              this.readerActive = false;
-            } else {
-              this.readerActive = true;
-            }
+          let currentURL = this.browser.currentURI.specIgnoringRef;
+
+          // Do nothing if there's no article or the page in this tab has changed.
+          if (article == null || (article.url != currentURL)) {
+            resetReaderFlags(currentURL);
             return;
           }
 
           this.savedArticle = article;
 
           Messaging.sendRequest({
             type: "Content:ReaderEnabled",
             tabID: this.id
           });
 
-          if(this.readerActive)
+          if (this.readerActive) {
             this.readerActive = false;
-
-          if(!this.readerEnabled)
+          }
+          if (!this.readerEnabled) {
             this.readerEnabled = true;
-        }, e => Cu.reportError("Error parsing document from tab: " + e));
+          }
+        }).catch(e => {
+          Cu.reportError("Error parsing document from tab: " + e);
+          resetReaderFlags(this.browser.currentURI.specIgnoringRef);
+        });
       }
     }
   },
 
   onStateChange: function(aWebProgress, aRequest, aStateFlags, aStatus) {
     let contentWin = aWebProgress.DOMWindow;
     if (contentWin != contentWin.top)
         return;