The default bug view has changed. See this FAQ.

merge nsIScriptError and nsIScriptError2 interfaces

VERIFIED FIXED in mozilla12

Status

Toolkit Graveyard
Error Console
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({addon-compat, dev-doc-complete})

Trunk
mozilla12
addon-compat, dev-doc-complete
Dependency tree / graph
Bug Flags:
in-testsuite -

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

5 years ago
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?).
(Assignee)

Comment 1

5 years ago
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

5 years ago
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.
(Assignee)

Comment 3

5 years ago
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?
Yes.
(Assignee)

Comment 5

5 years ago
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).
Assignee: nobody → acelists
Status: NEW → ASSIGNED

Comment 6

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

Comment 7

5 years ago
Created attachment 582917 [details] [diff] [review]
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).
Attachment #582917 - Flags: review?(neil)

Comment 8

5 years ago
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

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

r=me on the rest of the patch.
Attachment #582917 - Flags: review?(neil) → review+
(Assignee)

Comment 10

5 years ago
Created attachment 583226 [details] [diff] [review]
v2 patch, merges interfaces and updates callers

Thanks for your hint, I updated that. Fixed some typos while there.
Attachment #582917 - Attachment is obsolete: true
Attachment #583226 - Flags: superreview?(bzbarsky)
Attachment #583226 - Flags: review?(neil)
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 :)
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.)
(Assignee)

Comment 13

5 years ago
In that same spot? Or should I check all files again?
(Assignee)

Comment 14

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

Comment 15

5 years ago
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.
(Assignee)

Comment 16

5 years ago
Created attachment 583320 [details] [diff] [review]
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)).
Attachment #583226 - Attachment is obsolete: true
Attachment #583226 - Flags: superreview?(bzbarsky)
Attachment #583226 - Flags: review?(neil)
Attachment #583320 - Flags: review?(neil)
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.
Attachment #583320 - Flags: review?(neil) → review+
(Assignee)

Comment 18

5 years ago
Created attachment 583539 [details] [diff] [review]
v4, fixes nsnull check

Carrying over r=neil.
Attachment #583320 - Attachment is obsolete: true
Attachment #583539 - Flags: superreview?(bzbarsky)
Attachment #583539 - Flags: review+
Keywords: dev-doc-needed
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.
Attachment #583539 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 20

5 years ago
(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?
Starting line 427 in the attached diff.
(Assignee)

Comment 22

5 years ago
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())
+                      )
+         ) {
How about this:

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

?
(Assignee)

Comment 24

5 years ago
Created attachment 583588 [details] [diff] [review]
v5, style fixes

As you wish.
Attachment #583539 - Attachment is obsolete: true
Attachment #583588 - Flags: superreview?(bzbarsky)
Attachment #583588 - Flags: review+
That looks like comment 22 not comment 23.
(Assignee)

Comment 26

5 years ago
Created attachment 583596 [details] [diff] [review]
v6, fixed

Yes, sorry, forgot to update.
Attachment #583588 - Attachment is obsolete: true
Attachment #583588 - Flags: superreview?(bzbarsky)
Attachment #583596 - Flags: superreview?(bzbarsky)
Comment on attachment 583596 [details] [diff] [review]
v6, fixed

sr=me
Attachment #583596 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/916876a3db97
Flags: in-testsuite-
Keywords: checkin-needed
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/916876a3db97
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Keywords: addon-compat
(Assignee)

Updated

5 years ago
Status: RESOLVED → VERIFIED
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.
Keywords: dev-doc-needed → dev-doc-complete
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.