Closed Bug 1119482 Opened 5 years ago Closed 5 years ago

Split HoldDropJSObjects out of nsCycleCollector into its own file

Categories

(Core :: XPCOM, defect)

32 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(4 files)

No description provided.
Assignee: nobody → continuation
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.
Attachment #8570145 - Flags: review?(bugs)
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
Attachment #8570146 - Flags: review?(bugs)
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.
Attachment #8570145 - Flags: review?(bugs) → review+
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.
Attachment #8570146 - Flags: review?(bugs) → review?(nfroyd)
hold/drop should be usable from external code, if possible.
Attachment #8570148 - Flags: review?(bugs) → review+
Attachment #8570149 - Flags: review?(bugs) → review+
Blocks: 1137536
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...
(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.
(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...
(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.
(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?
Flags: needinfo?(continuation)
(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
Flags: needinfo?(continuation)
Attachment #8570146 - Flags: review?(nfroyd) → review+
You need to log in before you can comment on or make changes to this bug.