368842 - Uncaught Javascript exception in microsummaryPicker.js when opening Add Bookmark dialog for page that isn't HTML or XML

Status: RESOLVED FIXED

(Firefox Graveyard :: Microsummaries, defect, P1)

Description

[Jason Chu]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061218 BonEcho/2.0.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.1) Gecko/20061218 BonEcho/2.0.0.1

Uncaught Javascript exception in microsummaryPicker.js when opening Add Bookmark dialog for page that isn't HTML or XML, e.g., FTP sites and TXT files.  This prevents other scripts from running (i.e., my OpenBook extension).  FTP sites give the following exception:

Error: uncaught exception: [Exception... "'<error>' when calling method: [nsIMicrosummaryService::getMicrosummaries]"  nsresult: "0x805303f4 (<unknown>)"  location: "JS frame :: chrome://browser/content/bookmarks/microsummaryPicker.js :: MSP_init :: line 156"  data: no]

and TXT files give the following exception:

Error: uncaught exception: [Exception... "'page is neither HTML nor XML' when calling method: [nsIMicrosummaryService::getMicrosummaries]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "JS frame :: chrome://browser/content/bookmarks/microsummaryPicker.js :: MSP_init :: line 156"  data: no]

Reproducible: Always

Steps to Reproduce:
1. Go to ftp:// URL or http://*.txt URL
2. Open Add Bookmark dialog
3. Close dialog
4. Check Error Console
Actual Results:  
Exception thrown

Expected Results:  
No exceptions

Comment 1

[Ria Klaassen (not reading all bugmail)]

Regression range errors: 2006-06-29:11 - 2006-06-29:13.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-29+10%3A00&maxdate=2006-06-29+14%3A00
Probably bug 339543.

Comment 2

[Ria Klaassen (not reading all bugmail)]

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a2pre) Gecko/20070130 Minefield/3.0a2pre

Comment 3

[Myk Melez [:myk] [@mykmelez]]

I see this happening.  Looking into a fix.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: minimal fix for non-Places builds

Here's a minimal fix for non-Places builds that disables the microsummary picker if the microsummary service throws an error while retrieving microsummaries.  I'll attach a minimal fix for Places builds shortly.

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: minimal fix for Places builds

Here's the equivalent minimal patch for Places builds.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Note: adding a bookmark via the dialog seems horked in Places builds at the moment, so you can't see the problem (or the fix) when adding bookmarks via the dialog.  But you can see it when editing existing bookmarks, so I added bookmarks for the following URLs via drag-and-drop and then opened their properties to reproduce the bug (and confirm the fix):

ftp://ftp.mozilla.org/
http://www.ietf.org/rfc/rfc3092.txt


Note: the ultimate fix would find a better way than exceptions for the microsummary service to communicate the summarizability of a bookmark.  But this is complicated by asynchronous determination of summarizability in the case of editing a bookmark that isn't currently loaded in a tab (because the service has to download the file to discover that it isn't HTML or XML).  The minimal fixes in these patches do the next best thing of trapping the exceptions and disabling the picker when they occur (synchronously anyway).

Comment 7

[Asaf Romano (gone)]

Comment on attachment 254243 [details] [diff] [review]
patch v1: minimal fix for non-Places builds

>Index: browser/components/bookmarks/content/microsummaryPicker.js
>===================================================================
>@@ -113,6 +113,8 @@
>     return null;
>   },
> 
>+  _enabled: null,

>+    if (this._enabled == null) {

I prefer |if ("enabled" in this)|.

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2 for both Places and RDF bookmarks

> I prefer |if ("enabled" in this)|.

Ok, I've switched to that syntax.  I also removed an unnecessary Components.utils.reportError.  And I bundled the fixes for both Places-based and RDF-based bookmarks into a single patch.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Oh, and patch v2 also fixes a bug in the RDF-based |enabled| getter, moving |this._enabled = true| inside the |if (!("_enabled" in this))| block (otherwise the second and subsequent attempts to get |enabled| would always return true even if the picker was supposed to be disabled).

Comment 10

[Asaf Romano (gone)]

hrm? it still looks like you always set this._enabled to true

Comment 11

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v3: really fixes "always enabled" problem

> hrm? it still looks like you always set this._enabled to true

Ah, indeed.  Here's a version that doesn't.  I've tested it on all the various dialog modes we check for (Add Bookmark vs. Bookmark Properties for livemarks, bookmarking multiple tabs, separators, folders, and multiple pages), and it now determines the correct enabled state for all of them.

Comment 12

[Asaf Romano (gone)]

Comment on attachment 254920 [details] [diff] [review]
patch v3: really fixes "always enabled" problem

>Index: browser/components/bookmarks/content/microsummaryPicker.js
>===================================================================
>   get enabled() {
>-    if (this._mode == ADD_BOOKMARK_MODE) {
>+    if (!("_enabled" in this)) {
>       // If we're adding a bookmark, we only have to worry about livemarks
>-      // and bookmarking multiple tabs.
>-      if ("feedURL" in gArg || gArg.bBookmarkAllTabs)
>-        return false;
>-    }
>-    else if (this._mode == EDIT_BOOKMARK_MODE) {
>+      // and bookmarking multiple tabs; otherwise, the picker is enabled.
>+      if (this._mode == ADD_BOOKMARK_MODE) {
>+        if ("feedURL" in gArg || gArg.bBookmarkAllTabs)
>+          this._enabled = false;
>+        else
>+          this._enabled = true;

make that |this._enabled = !("feedURL" in gArg || gArg.bBookmarkAllTabs)|


>+      else if (this._mode == EDIT_BOOKMARK_MODE) {
>+        var isLivemark = BookmarksUtils.resolveType(gResource) == "Livemark";
>+        var isSeparator = BookmarksUtils.resolveType(gResource) == "BookmarkSeparator";

Calling resolveType twice here isn't necessary.

>+        var isContainer = RDFCU.IsContainer(BMDS, gResource);

and it sure isn't a container if either it's one of the other options.

>+        if (isLivemark || isSeparator || isContainer)
>+          this._enabled = false;
>+        else
>+          this._enabled = true;

or |this._enabled = !(isLivemark || isSeparator || isContainer)|

>       // We should never get to this point, since we're only being used
>       // in the Add Bookmark and Bookmark Properties dialogs, but if we're here
>       // for some reason, be conservative and assume the picker is disabled.
>-      return false;
>+      else {
>+        this._enabled = false;
>+      }

nit: remove the braces.

>     }
>   
>-    // We haven't found a reason to disable the picker, so say it's enabled.
>-    return true;
>+    return this._enabled;
>   },
> 
>+  set enabled(newValue) { this._enabled = newValue },

setters should generally return their value, but for this case just remove the setter and set this._enabled directly.

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v4: addresses review comments

> make that |this._enabled = !("feedURL" in gArg || gArg.bBookmarkAllTabs)|

Ah, nice simplification, done.


> >+      else if (this._mode == EDIT_BOOKMARK_MODE) {
> >+        var isLivemark = BookmarksUtils.resolveType(gResource) == "Livemark";
> >+        var isSeparator = BookmarksUtils.resolveType(gResource) == "BookmarkSeparator";
> 
> Calling resolveType twice here isn't necessary.

Indeed.  Ok, I now call it once.


> >+        var isContainer = RDFCU.IsContainer(BMDS, gResource);
> 
> and it sure isn't a container if either it's one of the other options.

Right, good point.


> >+        if (isLivemark || isSeparator || isContainer)
> >+          this._enabled = false;
> >+        else
> >+          this._enabled = true;
> 
> or |this._enabled = !(isLivemark || isSeparator || isContainer)|

Yup.  I've done something similar, except I factored out calls to resolveType,  minimized the creation of temporary variables, and don't check if it's a container until I know it isn't a livemark or separator.

        var bookmarkType = BookmarksUtils.resolveType(gResource);
        this._enabled = !(bookmarkType == "Livemark" ||
                          bookmarkType == "BookmarkSeparator" ||
                          RDFCU.IsContainer(BMDS, gResource));


> nit: remove the braces.

Ok, removed here and for the initial conditional block, which also contains just a single line now.


> setters should generally return their value, but for this case just remove the
> setter and set this._enabled directly.

Ok, done.

Comment 14

[Asaf Romano (gone)]

Comment on attachment 255124 [details] [diff] [review]
patch v4: addresses review comments

r=mano

Comment 15

[Myk Melez [:myk] [@mykmelez]]

Checked in to the trunk:

Checking in browser/components/bookmarks/content/microsummaryPicker.js;
/cvsroot/mozilla/browser/components/bookmarks/content/microsummaryPicker.js,v  <--  microsummaryPicker.js
new revision: 1.7; previous revision: 1.6
done
Checking in browser/components/places/content/bookmarkProperties.js;
/cvsroot/mozilla/browser/components/places/content/bookmarkProperties.js,v  <--  bookmarkProperties.js
new revision: 1.29; previous revision: 1.28
done


It'd be good to get this fix onto the branch for inclusion in an upcoming security/stability Firefox update, and the patch applies cleanly to the branch, so requesting wanted status.