698250 - Use instanceof instead of try...catch QueryInterface in consoleBindings appendItem method

Status: RESOLVED WONTFIX

(Toolkit Graveyard :: Error Console, defect)

Description

[Alex Vincent [:WeirdAl]]

Attachment: patch

Venkman users will like this.

Comment 1

[Philip Chee]

Check that removing the try/catch block doesn't regress Bug 288544.

Comment 2

[Alex Vincent [:WeirdAl]]

(In reply to Philip Chee from comment #1)
> Check that removing the try/catch block doesn't regress Bug 288544.

Uh oh.  How?  By removing the QI check and replacing it with instanceof, the only way I know to create a dummy message that might throw is to do so through a component - getting the XPCOM wrapping that way.  That seems... heavy for this.

I realize this patch didn't have a testcase, and the original patch which fixed this would have landed probably well before we had much in the way of automated testing.  I'm at a loss, frankly.

Comment 3

[Philip Chee]

Me neither, just pointing out that the try/catch block is there for other reasons than the QI check. The old XPFE Error Console was already using instanceof. Perhaps now that we are using infallible malloc, OOM exceptions might not be such a problem now.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 570528 [details] [diff] [review]
patch

This changes behavior if the aObject is neither a nsIScriptError nor a nsIConsoleMessage (we'll no longer reach the appendMessage(aObject) fallback, since we won't throw). I don't know how necessary that fallback is, but we shouldn't get rid of it unintentionally (and we should keep the try/catch around if we don't expect it to be necessary).

Comment 5

[:Gavin Sharp [email: [e-mail]]]

(In reply to Gavin Sharp (use [e-mail] for email) from comment #4)
> (and we should keep the try/catch around if we don't expect it to be necessary).

*shouldn't

Comment 6

[:aceman]

Alex? Can you continue with the patch?

Comment 7

[Alex Vincent [:WeirdAl]]

Uh, I'd forgotten about this... if someone wants to drive it, go ahead.

Comment 8

[Brian Grinstead [:bgrins]]

The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.

Comment 9

[Zack Weinberg (:zwol)]

I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)

Comment 10

[Brian Grinstead [:bgrins]]

The Error Console feature was removed entirely from the tree in Bug 1278368 and the bugzilla component is now being removed. We’ve migrated bugs that seem to also affect the Browser Console into the devtools component, please move this over if it was missed.