1071102 - Remove include of nsRefPtr.h from nsAutoPtr.h

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Bug 1071100 will split nsRefPtr into its own header file, but leave nsAutoPtr.h including nsRefPtr.h.  Ideally, we want to remove that, because the classes are not really related.

To fix that, we need to add an include of nsRefPtr.h to any file that uses nsRefPtr, and remove any includes of nsAutoPtr.h from files that don't use nsAutoPtr.  This could be done in this bug, or in separate bugs blocking this one if it proves to be easy to work on it piecemeal.

There are currently 811 includes of nsAutoPtr.h in the tree, so this will be a pretty big patch.  I think the right way to do this is to automatically generate it: look at all files that include nsAutoPtr.h, then whether they contain any uses of nsRefPtr or nsAutoPtr, and change the nsAutoPtr.h include as appropriate based on that.  Given my past experience, this will reveal a ton of header bootlegging (say, file A uses nsRefPtr, but does not include nsAutoPtr.h, but instead it includes header B.h, which includes nsAutoPtr.h), so manual fixups will be required.  Perhaps this problem can be avoided using a smarter approach.

(nfroyd suggested this two-stage approach to the header split.)

Comment 1

[Andrew McCreight [:mccr8]]

To be sure that all instances of header bootlegging are fixed, you are going to want to do a non-unified build before uploading the final patch, by adding
  ac_add_options --disable-unified-compilation
to your mozconfig, and then doing a clobber build.  This may not be a great bug for somebody with a slow machine, as it is going to involve a lot of waiting for recompiles.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

The following Linux bash script seems to pretty much do the job, although I haven't actually finished compiling.  I'd expect it would need a few minor fixups.

Whenever I write a shell script that gets this complicated, I always wish I had done it in Python instead.

Notes to reusers: Obviously, make sure you fix Bugzilla's inserted newlines.  Change --exclude-dir=obj to match the name of your object dir.  Run from the root of your clone.  I don't guarantee it will work with anything other than GNU sed/grep.

#!/bin/bash
for file in $(grep -RIl 'nsAutoPtr\|nsAutoArrayPtr' --exclude-dir=obj * | grep '\.\(h\|cpp\)' | grep -v 'xpcom/base/nsAutoPtr\.h')
do
  echo $file
  new_includes=$( \
    grep -v '^#include' $file | \
    grep -o '\b\(nsAutoPtr\|nsRefPtr\|nsCOMPtr\|nsAutoArrayPtr\)\b' | \
    sed 's/Array//g;s!nsRefPtr!mozilla\/nsRefPtr!' | \
    sort -u | \
    sed "s/^/#include \"/;s/$/.h\"\\\\/;\$s/\\\\//"
  )
  sed -i "/^#include.*\"\(nsAutoPtr\|mozilla\/nsRefPtr\|nsCOMPtr\)\.h\"/d;" $file
  if [ -n "$new_includes" ]
  then
    sed -i "0,/^#include/s!!\
${new_includes}\\\

#include!" $file
  fi
done

I can't actually take this bug now, because I don't have time to finish it off, but if no one has taken it by late September I'll probably take a look at it again.

Comment 3

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Patch

I know you're not technically an owner of this component, but this probably doesn't need real review anyway.  My script needed only two fixups to compile locally (includes added within an #ifdef and an external "C").

https://treeherder.mozilla.org/#/jobs?repo=try&revision=476b3bcd4ebd

Comment 4

[Andrew McCreight [:mccr8]]

Are you okay with my reviewing this 1MB patch, Nathan? Feel free to take it if you want. ;)

Comment 5

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #4)
> Are you okay with my reviewing this 1MB patch, Nathan? Feel free to take it
> if you want. ;)

Yes, that's fine.  We should just make you an XPCOM peer.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8670391 [details] [diff] [review]
Patch

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

Something isn't quite right with your script. I started auditing the files by hand, and I noticed that accessible/base/FocusManager.h actually uses nsRefPtr.h, but you remove the include of nsAutoPtr without adding nsRefPtr. Your try push is also broken on a few platforms, with what looks like similar issues.

I hacked up a crude Python script that does something similar to your shell script to analyze the uses and includes. It doesn't actually make any changes, but feel free to build on it: https://github.com/amccreight/moz-source-tools/blob/master/refptrinclude.py

::: accessible/base/FocusManager.h
@@ -4,5 @@
>  
>  #ifndef mozilla_a11y_FocusManager_h_
>  #define mozilla_a11y_FocusManager_h_
>  
> -#include "nsAutoPtr.h"

This file actually uses nsRefPtr, so it needs to include it.

Comment 7

[Andrew McCreight [:mccr8]]

Looking at the B2G build failure, your patch is removing the include of nsAutoPtr.h, but the file uses nsAutoPtr.

Comment 8

[Andrew McCreight [:mccr8]]

Though in that instance it is only being used in a declaration as a &, so technically it the template could be forward declared without including nsAutoPtr.h. It is probably not worth making a script sophisticated enough to deal with that.

Comment 9

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: New fixup script

Thanks for the template, it helped a lot.  I just don't know the Python standard libraries well enough to write this sort of thing quickly.

Comment 10

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1 -- Fix header bootlegging

Surprisingly, this is the only file I've found that depended only on things included by the *Ptr.h headers.

Comment 11

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 -- Clean up nsAutoPtr includes

This patch is purely autogenerated by the fixup script, no manual fixes.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to :Aryeh Gregor (working until October 13) from comment #9)
> Thanks for the template, it helped a lot.  I just don't know the Python
> standard libraries well enough to write this sort of thing quickly.

I don't know them very well either. I usually just do a web search for something like "python reverse list" and some Stack Overflow answer will turn up. ;)

Comment 13

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3 -- Remove unneeded include from nsAutoPtr.h

https://treeherder.mozilla.org/#/jobs?repo=try&revision=eba231e9f71b

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8670901 [details] [diff] [review]
Part 1 -- Fix header bootlegging

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

Nice.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 8670904 [details] [diff] [review]
Part 3 -- Remove unneeded include from nsAutoPtr.h

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

::: xpcom/base/nsAutoPtr.h
@@ -7,5 @@
>  #ifndef nsAutoPtr_h
>  #define nsAutoPtr_h
>  
>  #include "nsCOMPtr.h"
> -#include "mozilla/nsRefPtr.h"

Hmm. What is this file using nsCOMPtr.h for? nsCOMPtr.h includes nsRefPtr.h, so we're still including it indirectly. Maybe nsCOMPtr_helper needs to be in its own header file, so that nsCOMPtr.h doesn't have to include nsRefPtr.h. Still, this bug is a good step forward.

Comment 16

[Andrew McCreight [:mccr8]]

Comment on attachment 8670903 [details] [diff] [review]
Part 2 -- Clean up nsAutoPtr includes

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

I think your patch creates some bootlegging. For instance, in HTMLElementAccessibles.h, your patch removes the include of nsRefPtr.h, but HTMLElementAccessibles.cpp does use nsRefPtr.h, so it should get an include added. (In some sense, this was not really bootlegging before, because the header for the .cpp included it.)

I think this stems from the second half of this line in your script:
  if (not toRemove and not toAdd) or ('nsAutoPtr' not in includes and 'nsAutoPtr' not in uses):

Why do you have that in there?

Your try run is orange on b2g again. It looks like the problem is that DBusUtils.h bootlegs nsISupportsImpl.h for NS_INLINE_DECL_THREADSAFE_REFCOUNTING via RefPtr.h, and somehow your patch happens to break that.

You should wait until Nathan's patches in bug 1207245 settle out before landing this, as that is probably going to interfere with this a bit, and it breaks some of the bootlegging which will cause more havok. He landed it but it got backed out for breaking something.

Thanks for working on this. It is a pretty tedious thing to fix.

Comment 17

[Andrew McCreight [:mccr8]]

(Let me know if you've exceeded your time budget for this project and I can finish it off.)

Comment 18

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Andrew McCreight [:mccr8] from comment #16)
> I think this stems from the second half of this line in your script:
>   if (not toRemove and not toAdd) or ('nsAutoPtr' not in includes and
> 'nsAutoPtr' not in uses):
> 
> Why do you have that in there?

To reduce the size of the patch and therefore the number of problems I have to fix.  But you're right, it doesn't work.

> Thanks for working on this. It is a pretty tedious thing to fix.

Fixing tedious things is a specialty of mine!  Making nsresult an enum class was immeasurably more tedious -- this is easy, it's mostly scriptable.  :)

Comment 19

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Fixup script v3

Comment 20

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Notable changes in new version -- affects files that don't deal with nsAutoPtr, and has more aggressive comment detection.  I manually checked all the files affected by the latter change and there were no false negatives.  More types of comments needed to be ignored to avoid trying to include nsRefPtr.h in files like AlreadyAddRefed.h (which causes Bad Things).

Comment 21

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 1, v2 -- Fix header bootlegging

Comment 22

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 2 -- Manual include fixups

Comment 23

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 3, v2 -- Clean up ns(Auto|COM|Ref)Ptr includes

Comment 24

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Attachment: Part 4, v2 -- Remove unneeded includes from nsAutoPtr.h

Comment 25

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 8671374 [details] [diff] [review]
Part 4, v2 -- Remove unneeded includes from nsAutoPtr.h

Seems we don't need the nsCOMPtr.h include after all.  Try looks green enough that I'm optimistic it will finish green.  Obviously before pushing to inbound I'll adjust for Nathan's work and rerun the script to check for any new fixes needed before pushing.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=073f4865e9c4

Comment 26

[Andrew McCreight [:mccr8]]

This is looking pretty good to me. Sorry for the delay. Nathan said it was okay if this lands ahead of in bug 1207245, if you prefer.

There are still a few remaining issues, I think, having to do with the placement of the #includes. It looks like they are getting inserted before the first #include in the file.

The first problem with this is that it breaks when people include Foo.h in Foo.cpp, in order to check that Foo.h has all of the necessary includes. One example of this is dom/bluetooth/bluedroid/BluetoothSocketMessageWatcher.cpp. A simple fix for this would be to insert it after the first include, rather than before.

The second problem is that a number of the includes are getting inserted inside system-specific #ifdef blocks, like "#if defined(MOZ_WIDGET_GTK)". This is happening in a number of files, such as dom/camera/GonkRecorderProfiles.h, dom/ipc/ContentChild.cpp, dom/media/MediaDecoderStateMachine.cpp and xpcom/base/nsCycleCollector.cpp, but not as many as the previous problem. It looks like things build, but it is a bit unfortunate to leave all of this junk around. Detecting this particular issue may be tricky, given that you have to ignore include guards in headers. I'm not sure what should be done about that.

Comment 27

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Andrew McCreight [:mccr8] from comment #26)
> There are still a few remaining issues, I think, having to do with the
> placement of the #includes. It looks like they are getting inserted before
> the first #include in the file.

For files with no existing *Ptr.h includes, yeah.
 
> The first problem with this is that it breaks when people include Foo.h in
> Foo.cpp, in order to check that Foo.h has all of the necessary includes. One
> example of this is
> dom/bluetooth/bluedroid/BluetoothSocketMessageWatcher.cpp. A simple fix for
> this would be to insert it after the first include, rather than before.

I don't understand what this breaks.

> The second problem is that a number of the includes are getting inserted
> inside system-specific #ifdef blocks, like "#if defined(MOZ_WIDGET_GTK)".
> This is happening in a number of files, such as
> dom/camera/GonkRecorderProfiles.h, dom/ipc/ContentChild.cpp,
> dom/media/MediaDecoderStateMachine.cpp and xpcom/base/nsCycleCollector.cpp,
> but not as many as the previous problem. It looks like things build, but it
> is a bit unfortunate to leave all of this junk around. Detecting this
> particular issue may be tricky, given that you have to ignore include guards
> in headers. I'm not sure what should be done about that.

It should be pretty easy to come up with a heuristic that ignores the include guards.

I'm not going to be working for the next six months, so if you (or anyone else) want to finish this up, please do.  Otherwise I guess I'll take it up in six months.

Comment 28

[Andrew McCreight [:mccr8]]

(In reply to :Aryeh Gregor (not working until April 10-May 8) from comment #27)
> I don't understand what this breaks.

Here is the scenario: Foo.h uses nsRefPtr.h, but does not include the nsRefPtr header. Foo.cpp includes Foo.h, after it includes nsRefPtr.h. Foo.cpp compiles just fine, but if somebody else includes Foo.h it may not compile. By putting Foo.h as the first include of Foo.cpp, then we can be sure that at least one place checks that Foo.h does not implicitly rely on extra includes from outside of the file.

> I'm not going to be working for the next six months, so if you (or anyone
> else) want to finish this up, please do.  Otherwise I guess I'll take it up
> in six months.

Alright, thanks. Hopefully I'll find some time in the next six months.

Comment 29

[Andrew McCreight [:mccr8]]

I guess realistically I'm not going to have time to do this in the near term.

Comment 30

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Nathan Froyd posted to dev-platform on February 25 saying that he plans to axe nsAutoPtr, à la bug 1229985, so this bug is probably not worth pursuing.

Comment 31

[Andrew McCreight [:mccr8]]

Yeah, I guess that's true. Time spent on this bug would be better spent working towards that.