571008 - ViewerRegistry should guard against runtime errors in entry filters

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: ViewerRegistry.js cleanup

Right now, in ViewerRegistry.js, we guard against syntax errors in the filters, but not against errors that occur while executing those filters. Given the review process, which kind errors are most likely to sneak through?

Comment 1

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: Wrap filter execution in a try ... catch and report any errors

Comment 2

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: ViewerRegistry.js cleanup mkII--fixed some mistakes in first attachment

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and move return out of catch

Comment 4

[Shawn Wilsher :sdwilsh]

Comment on attachment 451751 [details] [diff] [review]
ViewerRegistry.js cleanup mkII--fixed some mistakes in first attachment

>+  /**
>+   * Returns the absolute url where the xul file for a viewer can be found.
>+   *
>+   * @param aIndex
>+   *        The numerical index of the entry representing the viewer.
>+   * @return
>+   *        A string of the fully canonized url.
er, is that how we've been dong @return?  I thought we kept that on the same line...

r=sdwilsh

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 451753 [details] [diff] [review]
Wrap filter execution in a try ... catch and report any errors mkII--work off cleanup mkII and move return out of catch

r=sdwilsh

Comment 6

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #4)
> (From update of attachment 451751 [details] [diff] [review])
> >+  /**
> >+   * Returns the absolute url where the xul file for a viewer can be found.
> >+   *
> >+   * @param aIndex
> >+   *        The numerical index of the entry representing the viewer.
> >+   * @return
> >+   *        A string of the fully canonized url.
> er, is that how we've been dong @return?  I thought we kept that on the same
> line...

Well, I've written dom-inspector stuff like that plenty of times, but I'm not sure if any of those have gotten submitted for review/checked in yet. It looks like attachment 445241 [details] [diff] [review] for bug 179621 did that. It makes the description line up with the params' descriptions, which can't happen if the "@return" and its description are on the same line. (Notice that "return" is one char longer than "param".)

Comment 7

[Shawn Wilsher :sdwilsh]

(In reply to comment #6)
> Well, I've written dom-inspector stuff like that plenty of times, but I'm not
> sure if any of those have gotten submitted for review/checked in yet. It looks
> like attachment 445241 [details] [diff] [review] for bug 179621 did that. It makes the description line
> up with the params' descriptions, which can't happen if the "@return" and its
> description are on the same line. (Notice that "return" is one char longer than
> "param".)
Hrm, I must have missed that.  Not sure that that is the style anywhere else in mozilla trees.

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

All right, I'll change it. Is it all right if I change the aforementioned line changed in attachment 445241 [details] [diff] [review], too?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> All right, I'll change it. Is it all right if I change the aforementioned line
> changed in attachment 445241 [details] [diff] [review], too?
yes

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

Pushed with those changes:
http://hg.mozilla.org/dom-inspector/rev/535e5fbda65c (cleanup)
http://hg.mozilla.org/dom-inspector/rev/03cf7e93828f (real changes)