Last Comment Bug 233307 - deCOMtaminate nsIScript*
: deCOMtaminate nsIScript*
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
:
Mentors:
Depends on:
Blocks: deCOM
  Show dependency treegraph
 
Reported: 2004-02-06 13:55 PST by Johnny Stenback (:jst, jst@mozilla.com)
Modified: 2014-04-25 15:15 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
deCOMtaminate. (377.15 KB, patch)
2004-02-06 13:56 PST, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
deCOMtaminate (diff -w of the above). (355.56 KB, patch)
2004-02-06 13:57 PST, Johnny Stenback (:jst, jst@mozilla.com)
bryner: review+
bryner: superreview+
Details | Diff | Splinter Review

Description Johnny Stenback (:jst, jst@mozilla.com) 2004-02-06 13:55:33 PST
Lots of things that could be cleaned up in those interfaces... patch coming up.
Comment 1 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-06 13:56:39 PST
Created attachment 140762 [details] [diff] [review]
deCOMtaminate.
Comment 2 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-06 13:57:22 PST
Created attachment 140763 [details] [diff] [review]
deCOMtaminate (diff -w of the above).
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-06 15:58:14 PST
FYI, there's a couple of silly errors in this patch that makes it not compile
(from last-minute changes that were not compiled before the full-tree diffs were
made). Nothing that should affect reviews tho...
Comment 4 Brian Ryner (not reading) 2004-02-08 22:25:35 PST
Comment on attachment 140763 [details] [diff] [review]
deCOMtaminate (diff -w of the above).

>Index: caps/src/nsScriptSecurityManager.cpp
>-already_AddRefed<nsIScriptContext>
>+static nsIScriptContext *
> GetScriptContext(JSContext *cx)
> {
>-    nsIScriptContext *scriptContext;
>-    GetScriptContextFromJSContext(cx, &scriptContext);
>-
>-    return scriptContext;
>+    return GetScriptContextFromJSContext(cx);
> }
> 

Hm, you could just eliminate this in favor of changing call callers to use
GetScriptContextFromJSContext... is this just to save on code size by only
inlining GetScriptContextFromJSContext once?

>Index: content/base/src/nsContentUtils.cpp
>@@ -152,88 +152,79 @@ nsContentUtils::GetParserServiceWeakRef(
>+nsIScriptGlobalObject *
>+nsContentUtils::GetStaticScriptGlobal(JSContext* aContext, JSObject* aObj)
> {
>   if (!sXPConnect) {
>-    *aNativeGlobal = nsnull;
>-
>-    return NS_OK;
>+    return nsnull;
>   }
> 
>   JSObject* parent;
>   JSObject* glob = aObj; // starting point for search
> 
>   if (!glob)
>-    return NS_ERROR_FAILURE;
>+    return nsnull;
> 
>   while (nsnull != (parent = JS_GetParent(aContext, glob))) {
>     glob = parent;
>   }
> 
>   nsCOMPtr<nsIXPConnectWrappedNative> wrapped_native;
> 
>   nsresult rv =
>     sXPConnect->GetWrappedNativeOfJSObject(aContext, glob,
>                                            getter_AddRefs(wrapped_native));
>-  NS_ENSURE_SUCCESS(rv, rv);
>+  NS_ENSURE_SUCCESS(rv, nsnull);
> 
>   nsCOMPtr<nsISupports> native;
>   rv = wrapped_native->GetNative(getter_AddRefs(native));
>-  NS_ENSURE_SUCCESS(rv, rv);

Don't bother assigning to rv here if it's not going to be used for anything.

>Index: content/base/src/nsRange.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsRange.cpp,v
>retrieving revision 1.178
>diff -u -9 -p -w -r1.178 nsRange.cpp
>--- content/base/src/nsRange.cpp	3 Feb 2004 23:23:09 -0000	1.178
>+++ content/base/src/nsRange.cpp	6 Feb 2004 21:44:58 -0000
>@@ -2484,32 +2484,32 @@ nsRange::CreateContextualFragment(const 
>         // Just to compare, not to use!
>         result = secMan->GetSystemPrincipal(getter_AddRefs(sysPrin));
>         if (NS_SUCCEEDED(result))
>             result = secMan->GetSubjectPrincipal(getter_AddRefs(subjectPrin));
>         // If there's no subject principal, there's no JS running, so we're in system code.
>         // (just in case...null subject principal will probably never happen)
>         if (NS_SUCCEEDED(result) &&
>            (!subjectPrin || sysPrin.get() == subjectPrin.get())) {
>           nsIScriptGlobalObject *globalObj = document->GetScriptGlobalObject();
>+          JSContext* cx = nsnull;
> 
>-          nsCOMPtr<nsIScriptContext> scriptContext;
>           if (globalObj) {
>-            result = globalObj->GetContext(getter_AddRefs(scriptContext));
>-          }
>+            nsIScriptContext *scriptContext = nsnull;
>+            scriptContext = globalObj->GetContext();

Combine these into a single line.

> 
>-          JSContext* cx = nsnull;
>-          if (NS_SUCCEEDED(result) && scriptContext) {
>+            if (scriptContext) {
>             cx = (JSContext*)scriptContext->GetNativeContext();
>           }
>+          }
> 
>           if(cx) {
>-            ContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1", &result);
>-            if(NS_SUCCEEDED(result)) {
>+            ContextStack = do_GetService("@mozilla.org/js/xpc/ContextStack;1");
>+            if(ContextStack) {

Can you clean up the parentheses style to match the rest of the function while
you're here (space after 'if')?

>               result = ContextStack->Push(cx);
>             }
>           }
>         }
>       }
> 
>       nsDTDMode mode = eDTDMode_autodetect;
>       nsCOMPtr<nsIHTMLDocument> htmlDoc(do_QueryInterface(domDocument));
>       if (htmlDoc) {

>Index: dom/public/nsIJSEventListener.h
>+  virtual ~nsIJSEventListener()
>+  {
>+    NS_IF_RELEASE(mContext);
>+  }

This destructor should not need to be virtual unless someone will be calling
|delete| on a nsIJSEventListener pointer (which they'd better not be).

>Index: embedding/components/printingui/src/win/nsPrintingPromptService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/embedding/components/printingui/src/win/nsPrintingPromptService.cpp,v
>retrieving revision 1.12
>diff -u -9 -p -w -r1.12 nsPrintingPromptService.cpp
>--- embedding/components/printingui/src/win/nsPrintingPromptService.cpp	19 Nov 2003 06:34:45 -0000	1.12
>+++ embedding/components/printingui/src/win/nsPrintingPromptService.cpp	6 Feb 2004 21:45:20 -0000
>@@ -135,24 +135,21 @@ nsPrintingPromptService::GetHWNDForDOMWi
>         {
>             HWND w;
>             site->GetSiteWindow(reinterpret_cast<void **>(&w));
>             return w;
>         }
>     }
> 
>     // Now we might be the Browser so check this path
>     nsCOMPtr<nsIScriptGlobalObject> scriptGlobal(do_QueryInterface(aWindow));
>-    nsCOMPtr<nsIDocShell> docShell;
>-    scriptGlobal->GetDocShell(getter_AddRefs(docShell));
>-    if (!docShell) return nsnull;
> 
>     nsCOMPtr<nsIDocShellTreeItem> treeItem;
>-    treeItem = do_QueryInterface(docShell);
>+    treeItem = do_QueryInterface(scriptGlobal->GetDocShell());

Maybe do this all on one line.

>Index: extensions/inspector/base/src/inLayoutUtils.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/extensions/inspector/base/src/inLayoutUtils.cpp,v
>retrieving revision 1.33
>diff -u -9 -p -w -r1.33 inLayoutUtils.cpp
>--- extensions/inspector/base/src/inLayoutUtils.cpp	22 Oct 2003 06:09:40 -0000	1.33
>+++ extensions/inspector/base/src/inLayoutUtils.cpp	6 Feb 2004 21:45:23 -0000
>@@ -77,20 +77,19 @@ inLayoutUtils::GetWindowFor(nsIDOMDocume
>   
>   nsCOMPtr<nsIDOMWindowInternal> window = do_QueryInterface(view);
>   return window;
> }
> 
> nsIPresShell* 
> inLayoutUtils::GetPresShellFor(nsISupports* aThing)
> {
>   nsCOMPtr<nsIScriptGlobalObject> so = do_QueryInterface(aThing);
>-  nsCOMPtr<nsIDocShell> docShell;
>-  so->GetDocShell(getter_AddRefs(docShell));
>+  nsIDocShell *docShell = so->GetDocShell();
>   
>   nsCOMPtr<nsIPresShell> presShell;
>   docShell->GetPresShell(getter_AddRefs(presShell));
> 

Since this isn't being null checked you could just chain the calls.

Just a general comment - I'm wondering about the efficiency of a few places
where you QI into a nsCOMPtr and then return it as a raw pointer.  If you QI'd
it into a raw pointer and returned it as |already_AddRefed|, you'd save an
addref and release if the caller is putting the value into a nsCOMPtr.	On the
other hand, it may increase code size a bit since you'll be spreading out
Release() calls to call sites that don't need to now.  Just something to
consider; do what you think is best.

r+sr=bryner with those changes.
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-09 12:24:49 PST
(In reply to comment #4)
> (From update of attachment 140763 [details] [diff] [review])
> >Index: caps/src/nsScriptSecurityManager.cpp
> >-already_AddRefed<nsIScriptContext>
> >+static nsIScriptContext *
> > GetScriptContext(JSContext *cx)
> > {
...
> >+    return GetScriptContextFromJSContext(cx);
> > }
> > 
> 
> Hm, you could just eliminate this in favor of changing call callers to use
> GetScriptContextFromJSContext... is this just to save on code size by only
> inlining GetScriptContextFromJSContext once?

Yup, just saving code size here, so I'll leave it as is...

...
> Just a general comment - I'm wondering about the efficiency of a few places
> where you QI into a nsCOMPtr and then return it as a raw pointer.  If you QI'd
> it into a raw pointer and returned it as |already_AddRefed|, you'd save an
> addref and release if the caller is putting the value into a nsCOMPtr.	On the
> other hand, it may increase code size a bit since you'll be spreading out
> Release() calls to call sites that don't need to now.  Just something to
> consider; do what you think is best.

Yeah, the bigger point of this patch was to reduce code size, performance isn't
much of an issue in most of this code. So I'd rather not spread out code for
nsCOMPtr<> dtor calls.

I fixed the rest.

Thanks for the reviews!
Comment 6 riscky 2004-02-16 12:23:50 PST
This bug has been fixed/checked in right? I only ask because my builds are now
broken. I haven't been about to produce any Firebird/Firefox (trunk) builds
since the 9th. I get the following error:

nsSecureBrowserUIImpl.cpp: In member function `virtual nsresult 
   nsSecureBrowserUIImpl::Init(nsIDOMWindow*)':
nsSecureBrowserUIImpl.cpp:196: error: no matching function for call to `
   nsDerivedSafe<nsIScriptGlobalObject>::GetDocShell()'
../../../../dist/include/dom/nsIScriptGlobalObject.h:73: error: candidates are: 
   virtual nsresult nsIScriptGlobalObject::GetDocShell(nsIDocShell**)
make[5]: *** [nsSecureBrowserUIImpl.o] Error 1


Looks, to me this check-in broke my build settings... no other builds seem to
exibit the behavior http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2004-02-16 13:43:28 PST
(In reply to comment #6)
...
> nsSecureBrowserUIImpl.cpp:196: error: no matching function for call to `
>    nsDerivedSafe<nsIScriptGlobalObject>::GetDocShell()'
> ../../../../dist/include/dom/nsIScriptGlobalObject.h:73: error: candidates are: 
>    virtual nsresult nsIScriptGlobalObject::GetDocShell(nsIDocShell**)
> make[5]: *** [nsSecureBrowserUIImpl.o] Error 1

Looks like your nsIScriptGlobalObject.h file is not up to date, update in
mozilla/dom and make sure you're getting the latest version (3.22 or later).

> Looks, to me this check-in broke my build settings... no other builds seem to
> exibit the behavior http://tinderbox.mozilla.org/showbuilds.cgi?tree=Firefox

Looks to me like your system either has old files laying around, or that your
tree is not up to date.

Email me (jst@mozilla.org) if you've got more questions...
Comment 8 Ryan VanderMeulen [:RyanVM] 2005-11-04 08:57:31 PST
Is there more needing to be done here or should this be marked fixed?
Comment 9 Zack Weinberg (:zwol) 2010-02-08 18:24:02 PST
Should this bug still be open?
Comment 10 :Ms2ger (⌚ UTC+1/+2) 2011-11-24 10:32:30 PST
I say no.

Note You need to log in before you can comment on or make changes to this bug.