239202 - nsIScriptGlobalObjectOwner.getScriptGlobalObject() should be non-scriptable

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Wladimir Palant]

nsIScriptGlobalObjectOwner.getScriptGlobalObject() returns nsIScriptGlobalObject
which isn't accessible from JavaScript so it should have [noscript]. Currently
calling this method produces an exception:

Error: uncaught exception: [Exception... "Cannot find interface information for
parameter arg 0 [nsIScriptGlobalObjectOwner.getScriptGlobalObject]"  nsresult:
"0x80570006 (NS_ERROR_XPC_CANT_GET_PARAM_IFACE_INFO)"

Comment 1

[Wladimir Palant]

Attachment: Patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Seems like this entire interface could be made non-scriptable. There is no js in
the tree that is calling it and trev suggested that the consoleservice could be
used which seems like a reasonable idea to me.

I definitly don't have any strong oppinions, i just dislike old code that
lingers around because 'it might be usefull some day to someone'

Comment 3

[Johnny Stenback (:jst)]

Hmm, while we're at it, maybe we want to get rid of the IDL completely and
deCOMtaminate this internal interface too? I think I'd prefer that...

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

trev: any news here? Didn't you have a patch that does what jst suggested in
comment 3?

Comment 5

[Wladimir Palant]

Yes, I have a patch but somehow it makes my build unstable and I didn't have the
time to find the reason yet. I'll try to do it the next days.

Comment 6

[Johnny Stenback (:jst)]

Wanna post what you have any maybe we can help you sort out what's wrong...?

Comment 7

[Wladimir Palant]

Attachment: Part 1 - Kill nsIScriptGlobalObjectOwner.reportScriptError()

I am splitting this patch into three independent parts so I can locate the
problem easier. This part removes reportScriptError() from the interface and
the only caller now uses nsIConsoleService directly. Tested, works without
problems.

Comment 8

[Wladimir Palant]

Attachment: Part 2 - Remove unnecessary dependancies

There are some files including nsIScriptGlobalObjectOwner.h without using it -
this patch fixes it (everything else are only whitespace changes).

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 146181 [details] [diff] [review]
Part 1 - Kill nsIScriptGlobalObjectOwner.reportScriptError()

lol, this is awesome code :)

r=me

Comment 10

[Johnny Stenback (:jst)]

Comment on attachment 146181 [details] [diff] [review]
Part 1 - Kill nsIScriptGlobalObjectOwner.reportScriptError()

Very nice :-) sr=jst

Comment 11

[Wladimir Palant]

Attachment: Part 3 - remove nsIScriptGlobalObjectOwner.idl

This patch removes the IDL interface (the new file nsIScriptGlobalObjectOwner.h
goes into the next attachment, I couldn't find a way to include it into the
patch). The new signature for GetScriptGlobalObject is |nsIScriptGlobalObject*
GetScriptGlobalObject()| which is the same as in nsIDocument. Maybe nsIDocument
should extend nsIScriptGlobalObjectOwner instead of defining the same method
again but that's a different issue.

This patch seems to work fine, maybe because I applied it to a clean tree now
or maybe I fixed the problem when doing some minor changes.

Comment 12

[Wladimir Palant]

Attachment: New file - /mozilla/dom/public/nsIScriptGlobalObjectOwner.h

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 146264 [details] [diff] [review]
Part 3 - remove nsIScriptGlobalObjectOwner.idl

- In nsXULPrototypeDocument::GetScriptGlobalObject():

+    if (!mGlobalObject) {
+	 nsresult rv = NewXULPDGlobalObject(getter_AddRefs(mGlobalObject));
+	 if (NS_FAILED(rv))
+	     return nsnull;

You could just remove this check alltogether, or if you feel more comfortable
with checking for an error (if you think NewXULPDGO() might ever return an
error, *and* a non-null out param, which would be a bug on its own) you should
null out mGlobalObject in case of a failure and fall through to the return at
the end.

sr=jst

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 146265 [details]
New file - /mozilla/dom/public/nsIScriptGlobalObjectOwner.h

>/* -*- Mode: IDL; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 2 -*-

This should be C++ now, not IDL. And make the tab width 2.

#define NS_ISCRIPTGLOBALOBJECTOWNER_IID \
  {0x413e8400, 0xa87f, 0x11d3, \
    { 0xaf, 0xc6, 0x00, 0xa0, 0x24, 0xff, 0xc0, 0x8c }}

You should generate a new IID for this interface since you're changing the
signature.

r+sr=jst

Comment 15

[Wladimir Palant]

Attachment: /mozilla/dom/public/nsIScriptGlobalObjectOwner.h with jst's corrections

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 146264 [details] [diff] [review]
Part 3 - remove nsIScriptGlobalObjectOwner.idl

>+nsIScriptGlobalObject*
>+nsXULPrototypeDocument::GetScriptGlobalObject()
> {
>-    nsresult rv = NS_OK;
>-    if (!mGlobalObject)
>-         rv = NewXULPDGlobalObject(getter_AddRefs(mGlobalObject));
>-    *_result = mGlobalObject;
>-    NS_IF_ADDREF(*_result);
>-    return rv;
>+    if (!mGlobalObject) {
>+        nsresult rv = NewXULPDGlobalObject(getter_AddRefs(mGlobalObject));
>+        if (NS_FAILED(rv))
>+            return nsnull;
>+    }
>+
>+    return mGlobalObject;

You don't need to do the |NS_FAILED(rv)| check, mGlobalObject has to be null
anyway, otherwise the next call to this function would fail.

In general, I don't like patches that does sweeping cleanups such as
whitespace-cleanups combined with actual functional changes. This makes the
patch a lot harder to read and increases to risk of regressions.

r=me

Comment 17

[Wladimir Palant]

Yes, I've already removed this check after jst commented on it. The current code is:

>-    nsresult rv = NS_OK;
>-    if (!mGlobalObject)
>-         rv = NewXULPDGlobalObject(getter_AddRefs(mGlobalObject));
>-    *_result = mGlobalObject;
>-    NS_IF_ADDREF(*_result);
>-    return rv;
>+    if (!mGlobalObject)
>+        NewXULPDGlobalObject(getter_AddRefs(mGlobalObject));
>+
>+    return mGlobalObject;

Do you want me to attach the corrected patch?

As to the whitespaces: sorry about this, it's my editor. Maybe I can find a
setting to disable conversion of tabs into whitespaces (though it's usually not
a big deal, most files contain only a few tabs if any).

Comment 18

[Wladimir Palant]

Attachment: Patch to be checked in

This is the patch combining all three parts and jst's correction.

Comment 19

[Wladimir Palant]

Attachment: Updated patch to be checked in

Got rid of bitrot and removed whitespace cleanups, hopefully this time the
patch will be checked in.

Comment 20

[Johnny Stenback (:jst)]

Checked in.

Comment 21

[Wladimir Palant]

Checked build 2004071809, nsIScriptGlobalObjectOwner isn't exposed through XPCOM
any more. Marking fixed.