904826 - Remove nsLayoutStatics::AddRef/Release calls that are no longer needed

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

These used to be needed to make sure XPConnect didn't go away while things were still in nsJSHolders, but khuey's refactoring for CC in workers means that isn't needed any more for that purpose.

Comment 1

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Bonus points if we create an RAII class for addref/releasing layout statics and replace all the needed references with that.

Comment 2

[Andrew McCreight [:mccr8]]

nsDOMFileReader looks like it may do this

Comment 3

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

file reader, xhr, websocket, event source, archive reader, and archive request were the ones I saw that looked like they could obviously go.

Comment 4

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Kyle Huey [:khuey] ([e-mail]) from comment #1)
> Bonus points if we create an RAII class for addref/releasing layout statics
> and replace all the needed references with that.

There already exists one in nsLayoutStatics.h - nsLayoutStaticsRef

Comment 5

[Andrew McCreight [:mccr8]]

Attachment: basic patch, untested

Here's a basic patch, fwiw.  I think the Lazy Content Unbinder one can also go.

Comment 6

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Mine was a little overreaching than yours, and things went really wrong.
https://tbpl.mozilla.org/?tree=Try&rev=84ab17c6f16a

Do you want to push to try?

Comment 7

[Andrew McCreight [:mccr8]]

Sure, I'll do that.  Looking at a few of the stacks, it looks like the problem with your push was just the WorkerRunnable thing.

Comment 8

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=90633757474e

Comment 9

[Andrew McCreight [:mccr8]]

nsContentUtils::WrapNative can certainly be cleaned up a bit.  I'm pretty sure that AddRefing XPConnect from off the main thread is verboten now, so that whole codepath can go.  I also expect that the reason the statics stuff exists is for the CC, so maybe that will be okay to remove, too.

Comment 10

[Andrew McCreight [:mccr8]]

Good point about the unused includes.  I've removed them locally, too.  I'll upload an updated patch when I'm sure it compiles.

Comment 11

[Andrew McCreight [:mccr8]]

Attachment: remove some more

This removes layout statics from a few headers, but ends up having to add nsThreadUtils to some other header, so maybe that's not a win over all.  I might be able to track down the files that use the macros and directly include there, though that's a bit gross.

Comment 12

[Andrew McCreight [:mccr8]]

I left the AddRef/Release calls in nsNodeInfoManager, though maybe they can be eliminated, too.

I also left them being in nsGlobalWindow and workers/RuntimeService.cpp, as those seem like fairly "top level" structures that may legitimately want to hold layout statics stuff alive.  Nikhil attempted to remove the latter in his try run, and it make things quite orange.  The problem seemed to be dom::workers::WorkerRunnable::Run() calling nsContentUtils::GetSafeJSContext() during shutdown.

I left behind the uses in nsStyleSheetService.cpp and nsLayoutModule.cpp because I didn't understand them, and that is actually in the layout directory, so maybe it is related.

Comment 13

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=72b6a6804745

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 793104 [details] [diff] [review]
remove some more

try run is green.  bholley for the changes in nsContentUtils.cpp, smaug for the rest.

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 793104 [details] [diff] [review]
remove some more

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

XPConnect is definitely alive until we invoke DestroyXPConnectSingleton() in the xpc module destructor, which should be invoked relatively late in XPCOM shutdown. So this should be fine.

r=bholley on the nsContentUtils changes.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 793104 [details] [diff] [review]
remove some more

This is somewhat risky, so please push try: -a

Comment 17

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=7c5aa24ded6d

Comment 18

[Andrew McCreight [:mccr8]]

Thanks for the reviews.

https://hg.mozilla.org/integration/mozilla-inbound/rev/365053e73efa

Comment 19

[Andrew McCreight [:mccr8]]

backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/baba4649d3b3

Bindings code stopped building, due to NS_IsMainThread not being declared, so I assume some recent header changes caused some code to start bootlegging through one of the layout statics headers I removed... Some bindings codegen will need to include nsThreadUtils now.

Comment 20

[Andrew McCreight [:mccr8]]

There are three places in Codegen.py that generate NS_IsMainThread.  I'll try to figure out under what conditions they are generated, and extend the include generation stuff to include nsThreadUtils in those cases.

Comment 21

[Andrew McCreight [:mccr8]]

Maybe I'll just include nsThreadUtils in BindingUtils.h, and deal with improving CodeGen in a followup.

Comment 22

[Andrew McCreight [:mccr8]]

Attachment: fix bindings build problems in a hacky way

The problem here is that various bindings code uses NS_IsMainThread(), but with my removal of the include of nsLayoutStatics from CallbackObject.h, nothing defines that.  This patch just adds an include of nsThreadUtils.h to BindingUtils.h, which seems a little more precise than CallbackObject.h, and includes the latter anyways, so it isn't any worse.

bz, you've been looking at bindings include code, so I'm flagging you for review on this.  I'll land this folded with the previous patch.  I can file a followup about being smarter in .cpp codegen about when we should include nsThreadUtils or not, but it didn't look trivial, and I'd like to get this landed before it bitrots further with the current #includeapalooza.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 796258 [details] [diff] [review]
fix bindings build problems in a hacky way

Yeah, r=me.

We're going to need this for any .cpp file with non-worker descriptors that have any methods or attributes, and for any file with chromeonly stuff, and any file with a dictionary.  Followup for narrowing this down further is fine, especially because it'll be easier once bug 909639 lands.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Actually, the other simple option is to throw this into the always-included headers in codegen and thus hide from non-codegen includers of BindingUtils.  The only issue with that is that it'll conflict with bug 909639.  So just make sure a followup is filed?

Comment 25

[Andrew McCreight [:mccr8]]

Filed 909971 for removing the include from BindingUtils.

https://hg.mozilla.org/integration/mozilla-inbound/rev/e4863a504838

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e4863a504838