1119482 - Split HoldDropJSObjects out of nsCycleCollector into its own file

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Use CycleCollectedJSRuntime::Get() in the various CC wrapper methods.

It is a little cleaner to use this helper method if we only care about the CCJSRuntime pointer,
and it will let us move some of these methods out of this file more easily.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Move HoldDropJSObjects.h from xpcom/glue to xpcom/base.

This will clear the way for a HoldDropJSObjects.cpp, as xpcom/glue seems to use weird linking rules.

try run isn't completely done but it should be okay:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c54ebf460307

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 3 - Move the Hold and DropJSObjects implementations to a new HoldDropJSObjects.cpp file.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 4 - Move IsJSHolder into HoldDropJSObjects.

Comment 5

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

Comment on attachment 8570145 [details] [diff] [review]
part 1 - Use CycleCollectedJSRuntime::Get() in the various CC wrapper methods.

Please don't use auto* here. It just makes the code harder to read.

Comment 6

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

Comment on attachment 8570146 [details] [diff] [review]
part 2 - Move HoldDropJSObjects.h from xpcom/glue to xpcom/base.

Hmm, this needs nathan's review I think.

Comment 7

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

hold/drop should be usable from external code, if possible.

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8570146 [details] [diff] [review]
part 2 - Move HoldDropJSObjects.h from xpcom/glue to xpcom/base.

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

It's not obvious to me what the payoff is here, I guess bug 1137517 is really what this is all about?  That seems like a worthy goal.

Could you elaborate on "xpcom/glue/ has weird linking rules" (which I agree, it does), and why exactly that causes problems?  I mean, there are other cycle collector .cpps in glue/, and those appear to be OK.  There's even glue->base references because of HoldDropJSObjects.h now...

Comment 9

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> It's not obvious to me what the payoff is here, I guess bug 1137517 is
> really what this is all about?  That seems like a worthy goal.

I'm pulling things out of the cycle collector .cpp file that aren't directly related to the cycle collector.

> Could you elaborate on "xpcom/glue/ has weird linking rules" (which I agree,
> it does), and why exactly that causes problems?  I mean, there are other
> cycle collector .cpps in glue/, and those appear to be OK.  There's even
> glue->base references because of HoldDropJSObjects.h now...

I'll try it again locally and see what the error was.  I guess I could leave the header in glue/ with the cpp in base/ maybe if that's better.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8)
> > It's not obvious to me what the payoff is here, I guess bug 1137517 is
> > really what this is all about?  That seems like a worthy goal.
> 
> I'm pulling things out of the cycle collector .cpp file that aren't directly
> related to the cycle collector.

Are they likely to be used by non-cycle collector things?

> > Could you elaborate on "xpcom/glue/ has weird linking rules" (which I agree,
> > it does), and why exactly that causes problems?  I mean, there are other
> > cycle collector .cpps in glue/, and those appear to be OK.  There's even
> > glue->base references because of HoldDropJSObjects.h now...
> 
> I'll try it again locally and see what the error was.  I guess I could leave
> the header in glue/ with the cpp in base/ maybe if that's better.

That's sort of weird, too, but maybe not much weirder than the cycle collector split we have now...

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10)
> > I'm pulling things out of the cycle collector .cpp file that aren't directly
> > related to the cycle collector.
> 
> Are they likely to be used by non-cycle collector things?

There's two categories of cycle-collector related things.  First, there's a bunch of gunk a class needs to implement in order to participate in cycle collection.  This includes the various CYCLE_COLLECTION macros, plus Hold/Drop.  Hold/Drop are used to root C++ objects holding JS objects in the GC and CC. (Well, it isn't really "rooting" them in the CC, but the CC wants to look at them.)  Second, there's a bunch of functions used to actually initialize and run the cycle collector itself.  I'd like to make nsCycleCollector.h contain only those things, so random CC participants don't have to include it.  It isn't a big deal, but it makes the separation clearer.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> > Could you elaborate on "xpcom/glue/ has weird linking rules" (which I agree,
> > it does), and why exactly that causes problems?  I mean, there are other
> > cycle collector .cpps in glue/, and those appear to be OK.  There's even
> > glue->base references because of HoldDropJSObjects.h now...
> 
> I'll try it again locally and see what the error was.  I guess I could leave
> the header in glue/ with the cpp in base/ maybe if that's better.

Did you get a chance to play around with this and see what the problem was?

Comment 13

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #12)
> Did you get a chance to play around with this and see what the problem was?

When I move the HoldDropJS.{h,cpp} files to xpcom/glue/, on top of the whole patch stack here, I get the link time error:

Undefined symbols for architecture x86_64:
  "mozilla::IsJSHolder(void*)", referenced from:
      mozilla::dom::indexedDB::IDBRequest::SetResultCallback(mozilla::dom::indexedDB::IDBRequest::ResultCallback*) in Unified_cpp_dom_indexedDB0.o
  "mozilla::cyclecollector::HoldJSObjectsImpl(nsISupports*)", referenced from:
       ...
  "mozilla::cyclecollector::DropJSObjectsImpl(nsISupports*)", referenced from:
       ...
  "mozilla::cyclecollector::DropJSObjectsImpl(void*)", referenced from:
       ...
  "mozilla::cyclecollector::HoldJSObjectsImpl(void*, nsScriptObjectTracer*)", referenced from:
       ...
 ld: symbol(s) not found for architecture x86_64

Comment 14

[Andrew McCreight [:mccr8]]

The header is exported from base/ instead of glue/ so that should still work.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1da20013ecf6
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/a91f48050ad1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6efde76492ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6df619aca839

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1da20013ecf6
https://hg.mozilla.org/mozilla-central/rev/a91f48050ad1
https://hg.mozilla.org/mozilla-central/rev/6efde76492ae
https://hg.mozilla.org/mozilla-central/rev/6df619aca839