458300 - deCOMtaminate nsINameSpaceManager: part 2

Status: RESOLVED FIXED

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

Description

[(dormant account)]

rename nsINameSpaceManager and change the class to not inherit from nsISupports and s/nsCOMPtr/nsRefPtr/ for pointers containing it, etc

Comment 1

[Paul Richardson]

I'm looking at this bug, and I understand why s/nsCOMPtr/nsRefPtr is necessary.  What else would be required for this bug?  Are there calls to methods inherited from nsISupports that would need to be removed?

Comment 2

[Paul Richardson]

Attachment: basic patch

After making the basic changes described here, I am having trouble with do_GetService.  Where it used to be possible to assign the resulting nsGetServiceByContractID directly into a nsCOMPtr, this does not seem to work for nsRefPtr.  I know that for other assignments in to ncCOMPtr, changing to nsRefPtr requires adding a call to AddRef, or creating a new instance.  However, I don't think that will work here.  

I'm not sure how to extract a raw pointer to a nsINameSpaceManager from a nsGetServiceByContractID.

Comment 3

[Markus Stange [:mstange]]

https://wiki.mozilla.org/Gecko:DeCOMtamination_Algorithm

Comment 4

[Reuben Morais [:reuben]]

Attachment: 1 - Remove some unneeded includes of nsINameSpaceManager.h

Comment 5

[Reuben Morais [:reuben]]

Attachment: 2 - Merge nsINameSpaceManager and NameSpaceManagerImpl

Comment 6

[Reuben Morais [:reuben]]

Attachment: 3 - Rename nsINameSpaceManager.h to nsNameSpaceManager.h

I tried to keep things sorted, but made no attempt to fix files that had unordered includes.

Comment 7

[Reuben Morais [:reuben]]

Attachment: 4 - Remove XPCOM goop

I removed the XPCOM registration code and added a GetInstance() method for users.

Comment 8

[Reuben Morais [:reuben]]

Attachment: 4 - Remove XPCOM goop

Forgot to qref before attaching.

Comment 9

[Reuben Morais [:reuben]]

Attachment: 0 - Remove unneeded includes in the HTML5 parser

Those files have big "DO NOT EDIT, GENERATED" warnings, but I couldn't find the code that emits the boilerplate, so I assumed that part isn't generated. It's just a search and replace so I'm attaching this version anyway.

Comment 10

[Reuben Morais [:reuben]]

I had a green Try run for this code, but since I cleaned up some of this stuff and split it into smaller patches, here's a new run:

https://tbpl.mozilla.org/?tree=Try&rev=6178e929bee5

(old run is here: https://tbpl.mozilla.org/?tree=Try&rev=e68a581687df)

Comment 11

[Reuben Morais [:reuben]]

Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop

Flagging you on these because you reviewed bug 455595, feel free to redirect. Patches 1 and 3 are mechanical changes, mostly need a rubberstamp.

Comment 12

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

Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser

Review of attachment 8364626 [details] [diff] [review]:
-----------------------------------------------------------------

::: parser/html/nsHtml5AttributeName.h
@@ +26,1 @@
>   */

If you scroll up a little bit further, you'll see

 * THIS IS A GENERATED FILE. PLEASE DO NOT EDIT.

Comment 13

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

Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop

Review of attachment 8364623 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/base/public/moz.build
@@ +58,5 @@
>      'nsINodeInfo.h',
>      'nsINodeList.h',
>      'nsIScriptElement.h',
>      'nsIStyleSheetLinkingElement.h',
> +    'nsLineBreaker.h',

Wrong patch

::: content/base/src/nsNameSpaceManager.cpp
@@ +37,5 @@
>  
> +StaticAutoPtr<nsNameSpaceManager> nsNameSpaceManager::sInstance;
> +
> +/* static */
> +nsNameSpaceManager* nsNameSpaceManager::GetInstance() {

/* static */ nsNameSpaceManager*
nsNameSpaceManager::GetInstance()
{

@@ +44,5 @@
> +    if (sInstance->Init()) {
> +      ClearOnShutdown(&sInstance);
> +    } else {
> +      delete sInstance;
> +      sInstance = nullptr;

Looks like a double free to me.

Comment 14

[Henri Sivonen (:hsivonen)]

Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser

This is generated code, so it would be good to identify what pattern in the generator is useless. It seems like an accident on my part that the generator puts the same set of includes in both foo.h and foo.cpp that anyway includes foo.h. However, it's intentional for simplicity that except for the tree builder and the tokenizer, all the generated files have the same set of includes.

Will the nsINameSpaceManager.h header continue to be need to be included in .cpp files under parser/html/? Which ones?

Comment 15

[Reuben Morais [:reuben]]

(In reply to Henri Sivonen (:hsivonen) from comment #14)
> Will the nsINameSpaceManager.h header continue to be need to be included in
> .cpp files under parser/html/? Which ones?

Yes, the ones touched by patch 3:

parser/html/nsHtml5AttributeName.cpp
parser/html/nsHtml5ElementName.cpp
parser/html/nsHtml5HtmlAttributes.cpp
parser/html/nsHtml5MetaScanner.cpp
parser/html/nsHtml5StackNode.cpp
parser/html/nsHtml5StateSnapshot.cpp
parser/html/nsHtml5TreeBuilder.cpp
parser/html/nsHtml5UTF16Buffer.cpp

Comment 16

[Henri Sivonen (:hsivonen)]

(In reply to Reuben Morais [:reuben] from comment #15)
> (In reply to Henri Sivonen (:hsivonen) from comment #14)
> > Will the nsINameSpaceManager.h header continue to be need to be included in
> > .cpp files under parser/html/? Which ones?
> 
> Yes, the ones touched by patch 3:

OK. I suggest just renaming the include in the other generated files as well and filing a separate bug about the redundant includes in the generated code. Unless fixed in the generator, the changes will be overwritten in the future.

Comment 17

[Henri Sivonen (:hsivonen)]

Comment on attachment 8364626 [details] [diff] [review]
0 - Remove unneeded includes in the HTML5 parser

r- for the reason stated in the previous comment.

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 8364616 [details] [diff] [review]
2 - Merge nsINameSpaceManager and NameSpaceManagerImpl

- In nsContentUtils::Shutdown():

   NS_IF_RELEASE(sConsoleService);
   sXPConnect = nullptr;
   NS_IF_RELEASE(sSecurityManager);
-  NS_IF_RELEASE(sNameSpaceManager);

This causes a leak AFAICT.

The rest looks good, but r- due to the leak.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 8364619 [details] [diff] [review]
3 - Rename nsINameSpaceManager.h to nsNameSpaceManager.h

- In editor/composer/src/nsComposeTxtSrvFilter.cpp:

 #include "nsError.h"                    // for NS_OK
 #include "nsIContent.h"                 // for nsIContent
 #include "nsIDOMNode.h"                 // for nsIDOMNode
-#include "nsINameSpaceManager.h"        // for kNameSpaceID_None
+#include "nsNameSpaceManager.h"        // for kNameSpaceID_None

Make the comment line up again, please.

- In editor/libeditor/base/nsEditor.cpp:

 #include "nsIHTMLDocument.h"            // for nsIHTMLDocument
 #include "nsIInlineSpellChecker.h"      // for nsIInlineSpellChecker, etc
 #include "nsIMEStateManager.h"          // for nsIMEStateManager
-#include "nsINameSpaceManager.h"        // for kNameSpaceID_None, etc
+#include "nsNameSpaceManager.h"        // for kNameSpaceID_None, etc

Here too...

- In editor/libeditor/html/nsHTMLEditUtils.cpp:

 #include "nsIAtom.h"                    // for nsIAtom
 #include "nsIDOMHTMLAnchorElement.h"    // for nsIDOMHTMLAnchorElement
 #include "nsIDOMNode.h"                 // for nsIDOMNode
-#include "nsINameSpaceManager.h"        // for kNameSpaceID_None
+#include "nsNameSpaceManager.h"        // for kNameSpaceID_None

... and here.

Comment 20

[Johnny Stenback (:jst)]

Comment on attachment 8364623 [details] [diff] [review]
4 - Remove XPCOM goop

- In content/base/src/nsNameSpaceManager.cpp:
 
+StaticAutoPtr<nsNameSpaceManager> nsNameSpaceManager::sInstance;

Ah, this fixes the leak in the earlier patch. Please move the removal of the release of sNameSpaceManager to this patch.

r=jst with that.

Comment 21

[Johnny Stenback (:jst)]

Comment on attachment 8364616 [details] [diff] [review]
2 - Merge nsINameSpaceManager and NameSpaceManagerImpl

r+ on this one too with my previous comments addressed.

Comment 22

[Reuben Morais [:reuben]]

Attachment: Rename nsINameSpaceManager.h to nsNameSpaceManager.h in the htmlparser's translator

Doing this in this bug since it's a minor thing.

Comment 23

[Reuben Morais [:reuben]]

Attachment: 2 - DeCOMtaminate nsINameSpaceManager

I merged the two patches because it's pointless to add extra code to make the tree build and work with the first patch just to remove it in the second one. Carrying r=jst.

Comment 24

[Reuben Morais [:reuben]]

Attachment: Rename nsINameSpaceManager.h to nsNameSpaceManager.h in the parser files

Comment 25

[Reuben Morais [:reuben]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2a3ff17ca99e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5837f5e79047
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1b5ee8c5491a

https://hg.mozilla.org/projects/htmlparser/rev/8987599c49b3

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2a3ff17ca99e
https://hg.mozilla.org/mozilla-central/rev/5837f5e79047
https://hg.mozilla.org/mozilla-central/rev/1b5ee8c5491a