711721 - merge nsIScriptError and nsIScriptError2 interfaces

Status: VERIFIED FIXED

(Toolkit Graveyard :: Error Console, defect)

Description

[:aceman]

Spinoff from bug 122213 comment 64.

The two interfaces nsIScriptError and nsIScriptError2 could probably be merged into one in js/xpconnect/idl/nsIScriptError.idl . Currently, nsIScriptError2 is an extension of nsIScriptError (inherits from it?).

Comment 1

[:aceman]

I have found these occurrences of nsIScriptError2 in /mozilla:

user:mozilla$ grep -r -n nsIScriptError2 *
browser/devtools/scratchpad/scratchpad.js:339:                        createInstance(Ci.nsIScriptError2);
browser/devtools/webconsole/test/browser_webconsole_bug_603750_websocket.js:25:        !(aSubject instanceof Ci.nsIScriptError2)) {
browser/devtools/webconsole/HUDService.jsm:6526:        !(aSubject instanceof Ci.nsIScriptError2) ||
content/base/src/nsEventSource.cpp:1094:  nsCOMPtr<nsIScriptError2> errObj(
content/base/src/nsWebSocket.cpp:130:  nsCOMPtr<nsIScriptError2> errorObject(
content/base/src/nsContentUtils.cpp:2833:  nsCOMPtr<nsIScriptError2> errorObject =
content/xml/document/src/nsXMLDocument.cpp:404:      nsCOMPtr<nsIScriptError2> errorObject =
dom/base/nsJSEnvironment.cpp:383:        nsCOMPtr<nsIScriptError2> error2(do_QueryInterface(errorObject));
dom/base/nsDOMClassInfo.cpp:1730:  nsCOMPtr<nsIScriptError2> scriptError =
dom/workers/WorkerPrivate.cpp:1112:    nsCOMPtr<nsIScriptError2> scriptError =
image/src/Decoder.cpp:133:    nsCOMPtr<nsIScriptError2> errorObject =
js/xpconnect/src/xpcprivate.h:3945:                      public nsIScriptError2 {
js/xpconnect/src/nsScriptError.cpp:50:                              nsIScriptError2)
js/xpconnect/src/XPCComponents.cpp:2749:    nsCOMPtr<nsIScriptError2> scripterr(do_CreateInstance(NS_SCRIPTERROR_CONTRACTID));
js/xpconnect/src/XPCWrappedJSClass.cpp:1136:                                nsCOMPtr<nsIScriptError2> scriptError2 =
js/xpconnect/idl/nsIScriptError.idl:100:interface nsIScriptError2 : nsISupports {
layout/style/nsCSSScanner.cpp:427:    nsCOMPtr<nsIScriptError2> errorObject =
layout/style/nsFontFaceLoader.cpp:800:  nsCOMPtr<nsIScriptError2> scriptError =
parser/htmlparser/src/nsExpatDriver.cpp:950:    nsCOMPtr<nsIScriptError2> serr2(do_QueryInterface(serr));

There were none in /mailnews, /mail, /suite .

Is it enough to merge the interfaces into nsIScriptError (containing all the properties) and then just change all these places to use nsIScriptError ?

Comment 2

[[e-mail]]

Most of these locations need both nsIScriptError and nsIScriptError2 so in that case you would simply reuse the existing object, but otherwise yes.

I did wonder whether it's worth putting timeStamp back after innerWindowID, but it's probably not.

Comment 3

[:aceman]

I see a pattern like this:

  nsCOMPtr<nsIScriptError2> errorObject =
      do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);

  errorObject->InitWithWindowID(...);

  nsCOMPtr<nsIScriptError> logError = do_QueryInterface(errorObject);
  return sConsoleService->LogMessage(logError);

Can I change it to this?

  nsCOMPtr<nsIScriptError> errorObject =
      do_CreateInstance(NS_SCRIPTERROR_CONTRACTID, &rv);

  errorObject->InitWithWindowID(...);

  return sConsoleService->LogMessage(errorObject);

Is this the reuse you mean?

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Yes.

Comment 5

[:aceman]

Thanks, so I can try it.

Does this have any performance considerations?
In the pattern below, it seems it could save one pointer (or a full object?) and do_QueryInterface call.
On the other hand, places that were sufficient with nsIScriptError interface will now get the full merged interface (with the 2 variables added). But from my understanding this is no real change, as in the background they were already getting the full nsIScriptError object, just didn't have the interface to access those 2 variables (3 before bug 122213).

So should this be a theoretical performance win? Did I miss anything? Or do we not care as there are not many of these objects created (not many messages).

Comment 6

[[e-mail]]

Yes, it is technically both a performance and size win (the nsScriptError object has one fewer vtable pointer).

Comment 7

[:aceman]

Attachment: patch, merges interfaces and updates callers

This is my attempt at it. It does compile and work in Thunderbird fine. The error console is populated with some messages (that appear at each start).

Comment 8

[[e-mail]]

Comment on attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

>     // Otherwise log an error to the error console.
>-    nsCOMPtr<nsIScriptError2> scriptError =
>+    nsCOMPtr<nsIScriptError> scriptError =
>       do_CreateInstance(NS_SCRIPTERROR_CONTRACTID);
>     NS_WARN_IF_FALSE(scriptError, "Faild to create script error!");
> 
>     nsCOMPtr<nsIConsoleMessage> consoleMessage;
This one confused you by using nsIConsoleMessage instead of nsIScriptError, but they can in fact be the same variable too.

Comment 9

[[e-mail]]

Comment on attachment 582917 [details] [diff] [review]
patch, merges interfaces and updates callers

r=me on the rest of the patch.

Comment 10

[:aceman]

Attachment: v2 patch, merges interfaces and updates callers

Thanks for your hint, I updated that. Fixed some typos while there.

Comment 11

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 583226 [details] [diff] [review]
v2 patch, merges interfaces and updates callers

>diff --git a/dom/workers/WorkerPrivate.cpp b/dom/workers/WorkerPrivate.cpp
> ...
>     if (scriptError) {
>       if (NS_SUCCEEDED(scriptError->InitWithWindowID(aMessage.get(),
>                                                      aFilename.get(),
>                                                      aLine.get(), aLineNumber,
>                                                      aColumnNumber, aFlags,
>                                                      "Web Worker",
>                                                      aInnerWindowId))) {
>-        consoleMessage = do_QueryInterface(scriptError);
>-        NS_ASSERTION(consoleMessage, "This should never fail!");
>       }
>       else {
>         NS_WARNING("Failed to init script error!");
>       }
>     }

Drive-by, please don't leave an empty if clause. Switch the condition to NS_FAILED :)

Comment 12

[[e-mail]]

One other thing I spotted was that the new code tries to log the script error even if it failed to init it; the old one didn't because of the extra call to QueryInterface that only happened if the init succeeded. (Most users of LogMessage just bail out early if init fails, so it's not normally a problem.)

Comment 13

[:aceman]

In that same spot? Or should I check all files again?

Comment 14

[:aceman]

In that one spot, can I just set scriptError to nsnull so that the following tests ( if (scriptError) {} ) catch it?

Comment 15

[:aceman]

It seems I have found about 4 other places in the patch where InitWithWindowID was not checked even before my changes. I'll fix that too.

Comment 16

[:aceman]

Attachment: v3, error checking fixes

So I added the error checking of InitWithWindowID in the places I thought were missing it. I tried to guess from the surrounding code whether each individual function can return early with the error, or it should continue just without sending the error to the console.

I found another spot in js/xpconnect/src/XPCConvert.cpp (where I didn't touch anything previously) what is not checking InitWithWindowID. Maybe that should be fixed up, but I don't know if it can just return in case of error (NS_ENSURE_SUCCESS (rv,rv)).

Comment 17

[[e-mail]]

Comment on attachment 583320 [details] [diff] [review]
v3, error checking fixes

>+      if (scriptError != nsnull) {
Doesn't need to be explictly checked against nsnull: if (scriptError) is fine.

Comment 18

[:aceman]

Attachment: v4, fixes nsnull check

Carrying over r=neil.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 583539 [details] [diff] [review]
v4, fixes nsnull check

Please leave the curlies in the style system code and fix the odd "line up the '(' with the first letter of the function name" indent in the one place you use it, and sr=me.

Comment 20

[:aceman]

(In reply to Boris Zbarsky (:bz) from comment #19)
> fix the odd "line up
> the '(' with the first letter of the function name" indent in the one place
> you use it, and sr=me.

Not sure what you mean with this. What line number in the patch?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Starting line 427 in the attached diff.

Comment 22

[:aceman]

There is not much space left if we want to wrap at 80 columns. What about this:
+      if (NS_SUCCEEDED(errorObject->InitWithWindowID(msg.get(),
+                       NS_ConvertUTF8toUTF16(mImage.GetURIString()).get(),
+                       nsnull, 0, 0, nsIScriptError::errorFlag,
+                       "Image", mImage.InnerWindowID())
+                      )
+         ) {

Comment 23

[Boris Zbarsky [:bzbarsky]]

How about this:

+      if (NS_SUCCEEDED(errorObject->InitWithWindowID(
+                         msg.get(),
+                         NS_ConvertUTF8toUTF16(mImage.GetURIString()).get(),
+                         nsnull, 0, 0, nsIScriptError::errorFlag,
+                         "Image", mImage.InnerWindowID()
+                       ))) {

?

Comment 24

[:aceman]

Attachment: v5, style fixes

As you wish.

Comment 25

[Boris Zbarsky [:bzbarsky]]

That looks like comment 22 not comment 23.

Comment 26

[:aceman]

Attachment: v6, fixed

Yes, sorry, forgot to update.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 583596 [details] [diff] [review]
v6, fixed

sr=me

Comment 28

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/916876a3db97

Comment 29

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/916876a3db97

Comment 30

[Eric Shepherd [:sheppy]]

Documentation updated:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError2
https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsIScriptError

And mentioned on Firefox 12 for developers.