The JS engine has a notion of ZoneGroup that we plan to use as the unit of threading for Quantum DOM. We want to create one ZoneGroup per TabGroup so that the two concepts are 1:1. Until we actually implement scheduling for Quantum DOM, this change won't have much effect. But I have the code now, so we might as well land it.

The way I did this is kinda gross. I'm hoping you'll think of a better way :-).

Comment on attachment 8855084 [details] [diff] [review] patch Never mind. This is failing on try and I'll need to figure out why.

What's the interaction between zonegroups and zones? Presumably multiple zones per zonegroup, right? I would think we would still want zone-per-tab, but maybe we want zone-per-tabgroup? In any case, ideally we'd just store the ZoneGroup* on the TabGroup or something...

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #3) > What's the interaction between zonegroups and zones? Presumably multiple > zones per zonegroup, right? I would think we would still want zone-per-tab, > but maybe we want zone-per-tabgroup? Yeah, I screwed that up. I'm hoping if I fix that we won't crash. Although we still shouldn't be crashing. I filed bug 1354724 about that. > In any case, ideally we'd just store the ZoneGroup* on the TabGroup or > something... I'm a little worried about lifetimes. ZoneGroups get swept by the GC when all their zones are gone. We could null out a TabGroup's ZoneGroup when TabGroup::Leave is called for the last outer window. But I'm not sure we can guarantee that the ZoneGroup will live that long. Do you know if it will?

I don't offhand. :(

Well, however we do this, it needs to not crash. Brian, is there any chance you could look into this? We're getting GC crashes when I try to use zone groups: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ead0408dc60c9a33ac26e5323d3167e46b362983 All the crashes seem to be in devtools tests, and some of them have debugger-related marking or sweeping code on the stack.

Sure, I'll look into this. All the crashes I see in that try run seem to be at the same place, in jsweakmap.h / assertEntriesNotAboutToBeFinalized(). Are you seeing crashes in other places?

(In reply to Brian Hackett (:bhackett) from comment #7) > Sure, I'll look into this. All the crashes I see in that try run seem to be > at the same place, in jsweakmap.h / assertEntriesNotAboutToBeFinalized(). > Are you seeing crashes in other places? This crash happens in markIteratively (also in jsweakmap.h). https://treeherder.mozilla.org/logviewer.html#?job_id=89721965&repo=try&lineNumber=9466 Thanks!

This patch might fix the GC crashes, though I won't be able to test it until tomorrow. Cross compartment weak maps need to have their source and target zones swept in the same GC slice, but this isn't actually happening because the related debugger code assumes an old invariant (that debuggers and debuggees are in the same zone group) that is no longer guaranteed.

I pushed this to try (after fixing a small compile error): https://treeherder.mozilla.org/#/jobs?repo=try&revision=9725e800b6e282af12bcece4685974c29fa30996 Brian, should I be setting up the zone groups differently when debuggers are present? I haven't really thought much about how to handle debuggers (mostly since I don't understand much about how debugging actually gets enabled).

(In reply to Bill McCloskey (:billm) from comment #10) > I pushed this to try (after fixing a small compile error): > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=9725e800b6e282af12bcece4685974c29fa30996 > > Brian, should I be setting up the zone groups differently when debuggers are > present? I haven't really thought much about how to handle debuggers (mostly > since I don't understand much about how debugging actually gets enabled). Thanks! It looks to me like the devtools crashes are not happening anymore with the patch. I don't think you should be doing anything special about zone group membership when debuggers are present. There's nothing to keep a debugger from being created at some point in the future, and we definitely don't want to have to make changes to existing zone groups when a debugger is first created. The debugger can observe all activity happening in a runtime, and without making changes to how it works we need to allow it to create cross-compartment edges basically whenever it wants, including across zone groups. When I was working on the patches for bug 1323066 I had hoped that we would be able to keep debuggers in the same zone group as their debuggees, but later realized as above that this is not possible with how things are currently structured. The patch I attached earlier fixes a place where this old, invalid invariant was still being assumed.

Comment on attachment 8857232 [details] [diff] [review] potential crash fix Fix a place where we are assuming a ZoneGroup invariant that is not guaranteed to hold, see comment 11.

Pushed by bhackett@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/30f56ab08d4f Check all zone groups when adding edges for debugger cross compartment wrappers, r=jandem.

Thanks Brian.

This patch actually creates zone groups that make sense. Regarding lifetimes, I think that in this particular case it would probably be safe to store a ZoneGroup on the TabGroup. However, I like the fact that the API we expose from the JS engine is safe and doesn't even allow for lifetime screw-ups. So I stuck with the old approach from before, but cleaned it up. I think it's reasonably nice now. Brian, something has regressed this since the last try push. I get this assertion: Assertion failure: ionLazyLinkListSize_ > 0, at /home/billm/ws/gecko/js/src/gc/ZoneGroup.cpp:103 Presumably something in bug 1347539 broke it. Would you mind taking a look? It's very easy to reproduce. I just build a browser and visit a random site. It crashes after a few seconds.

Comment on attachment 8860589 [details] [diff] [review] patch, v3 >+static JS::CompartmentCreationOptions We're ending up making multiple copies of this struct, but I guess that's not too expensive compared to this general codepath... Still, it seems like we could have zero copies if we made this take a non-const CompartmentCreationOptions reference and called it from CreateNativeGlobalForInner instead of nsGlobalWindow::SetNewDocument. Is there a reason it's being called from SetNewDocument? >+ return options.setExistingZone(top->GetGlobalJSObject()); OK, so this is using the global of our top _outer_ window. >+ if (nsPIDOMWindowInner* inner = outer->GetCurrentInnerWindow()) { But this is trying to ensure that things have inner windows. Why? Also, it bothers me a bit that we're only examining GetTopLevelWindows(). What guarantees that those have inners any time anything inside them does? That's the invariant we're relying on here, right? I have no idea whether that invariant holds. Ideally we'd just check all the outer windows in the tabgroup, not just the toplevel ones. r=me with those bits fixed.

(In reply to Bill McCloskey (:billm) from comment #16) > Created attachment 8860589 [details] [diff] [review] > patch, v3 > > This patch actually creates zone groups that make sense. > > Regarding lifetimes, I think that in this particular case it would probably > be safe to store a ZoneGroup on the TabGroup. However, I like the fact that > the API we expose from the JS engine is safe and doesn't even allow for > lifetime screw-ups. So I stuck with the old approach from before, but > cleaned it up. I think it's reasonably nice now. > > Brian, something has regressed this since the last try push. I get this > assertion: > Assertion failure: ionLazyLinkListSize_ > 0, at > /home/billm/ws/gecko/js/src/gc/ZoneGroup.cpp:103 > > Presumably something in bug 1347539 broke it. Would you mind taking a look? > It's very easy to reproduce. I just build a browser and visit a random site. > It crashes after a few seconds. There's a patch to fix this in bug 1357367, which I'm going to try to land today.

Pushed by wmccloskey@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/b824955ccda3 Create ZoneGroups corresponding to TabGroups (r=bz)

Thanks Brian!