Closed Bug 816784 Opened 12 years ago Closed 7 years ago

Lots of time spent in WindowDestroyedEvent::Run/js::NukeCrossCompartmentWrappers when closing tabs/windows

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Project Flags:
Performance Impact high
Tracking Flags:
Tracking Status
firefox46 --- affected
firefox47 --- affected
firefox48 --- affected
firefox49 --- affected
firefox55 --- fixed

People

(Reporter: glandium, Assigned: ting)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Snappy:?][tw-dom])

Crash Data

Attachments

(6 files, 1 obsolete file)

nukeCCW.png
164.12 KB, image/png
Details
fix_bug_816784.patch
6.13 KB, patch
khuey
: review-
Details | Diff | Splinter Review
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.
59 bytes, text/x-review-board-request
jonco
: review+
Details
Bug 816784 part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers.
59 bytes, text/x-review-board-request
jonco
: review+
Details
Bug 816784 part 3 - Optimize the other places that iterate CCWs.
59 bytes, text/x-review-board-request
jonco
: review+
Details
Bug 816784 part 4 - Fix the rooting hazards that the analysis report.
59 bytes, text/x-review-board-request
sfink
: review+
Details 
I have 1367 tabs. Both when switching to private browsing and when shutting down the browser, a huge amount of time is spent on 
js::NukeCrossCompartmentWrappers, called from WindowDestroyedEvent::Run, hanging the UI for more than a minute.
Blocks: hueyfix
Component: General → DOM
Product: Firefox → Core
We're also doing this at startup now... bug 989816
Attached image nukeCCW.png — 
I'm also getting hit by this during normal browsing (in addition to massive startup slowdowns) with multi-second pauses in this method according to gecko profiler.
STR with 100 tabs:

- create a clean profile
- install greasemonkey 3.3, µmatrix 0.9.1.2, gecko profiler
- restart
- run this from the browser toolbox to open 100 tabs: for(let i=0;i<100;i++) window.openNewTabWith("about:blank");
- start profiling
- open http://vimcolorschemetest.googlecode.com/svn/html/index-c.html
- wait for it to finish loading, then reload, repeat a few times
- get profiler results

Here's my results: http://people.mozilla.org/~bgirard/cleopatra/#report=89e0a2780368e2050a12280d9c26fc0ab625ca14&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A335890,%22end%22%3A347283%7D%5D&selection=0,1,2,4247,4248,4209,4249,14,15,4250,4251,12,13,14,15,4250,16,18,7170,7171

As you can see the child process' CPU time is dominated by CCW-nuking for the selected time span.


> Platform: 	x86_64 Linux 

should probably be changed to all platforms.
Blocks: 1219586
Fix of this bug against Firefox 42.0.

The code is released under MPL 2.0, LGPL 2.1, GPL 2.0, AGPL 3.0 as well as any later version.
Attachment #8693031 - Flags: review?(bzbarsky)
I'm curious about performance numbers If I understand that patch correctly it improves the runtime of closing N tabs out of M from O(N*M) to O(M). Maybe that's good enough in practice, but I believe things could be optimized further by only visiting those compartments that ever held a CCW to the nuked ones. Although that would require a coarse-grained reverse lookup table.
(In reply to The 8472 from comment #6)
> I'm curious about performance numbers If I understand that patch correctly
> it improves the runtime of closing N tabs out of M from O(N*M) to O(M).

For a restore of a real-life session which consists of about 8000 tabs, without that change, the firefox process consumed 285 minutes of CPU time, while with that change, the firefox process consumed 55 minutes of CPU time.

Without that change, most of the CPU time would be spent in js::NukeCrossCompartmentWrappers(). With that change, that time is negligible.

(Note that these numbers come from a firefox build where the GC was tuned to kick in only seldomly, else https://bugzilla.mozilla.org/show_bug.cgi?id=1183797 would dominate all CPU time, anyway.)

> Maybe that's good enough in practice,

Apparently, it is.

> but I believe things could be
> optimized further by only visiting those compartments that ever held a CCW
> to the nuked ones. Although that would require a coarse-grained reverse
> lookup table.

Yes. But given there are many other much more pressing performance bugs (e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=945702 and a session restore time of still 55 minutes where almost all tabs are _not_ loaded, whereas a restore time of 60 seconds may be acceptable), the current fix seems to be "good enough".
Comment on attachment 8693031 [details] [diff] [review]
fix_bug_816784.patch

Sorry for the lag here; I was trying to make sure I understood this code correctly.

So the biggest issue I see is that closing a single window does absolutely nothing to clean it up.  You have to close 9 other windows before anything gets cleaned up.  This seems fairly suboptimal for the normal browsing case.  I would prefer it if we went ahead and did the cleanup after some time has passed since the last thing was enqueued to gPendingWindows, even if there aren't 10 windows in there yet.

Apart from that:

1)  Do we have any guarantees that the nsGlobalWindow can't die (nulling out the weakref) while its JSObject is still alive?  Especially in the outer window case, this worries me a bit.

2)  Why do we want to add to gPendingWindows from WindowDestroyedEvent::Run, as opposed to from whatever posts the WindowDestroyedEvent?  Are we trying to ensure that the nuking comes after the NotifyObservers call there?  I guess we do need that; it's worth documenting.

3)  Having a statically allocated nsCOMArray is not OK.  It needs to be heap-allocated, and deleted late enough in shutdown.  Or better yet, just put the list of windows directly in the WindowRequestCleanupEvent and then keep a pointer to it around so you (1) can append to it and (2) don't bother posting a new one while one is live.

4)  During js::NukeCrossCompartmentWrappers, don't compartments possibly get destroyed as you go?  And if so, what makes sure that new ones don't get allocated in their spot, confusing the matcher?  Seems like you need to keep those compartments alive until after NukeCrossCompartmentWrappers returns.

Past that, now that Kyle is back he's a much better reviewer for this than I am.
Attachment #8693031 - Flags: review?(bzbarsky) → review?(khuey)
Comment on attachment 8693031 [details] [diff] [review]
fix_bug_816784.patch

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

Sorry for the delay here.

Your editor has left trailing whitespace on a number of lines.  Please clean that up.

I agree with Boris that we should clean up less than 10 windows after a while on a timer.  We also should clean up any pending windows when the "memory-pressure" notification is fired.  I'll respond to Boris's other comments directly.

::: dom/base/nsGlobalWindow.cpp
@@ +54,4 @@
>  #include "mozilla/Preferences.h"
>  #include "mozilla/Likely.h"
>  #include "mozilla/unused.h"
> +#include "../../js/public/HashTable.h"

Just "js/HashTable.h" will work.

@@ +9167,5 @@
>    }
>  };
>  
> +class MultiCompartmentMatcher : public js::CompartmentFilter {
> +  friend class WindowRequestCleanupEvent;

I would prefer that you add relevant accessor methods on MultiCompartmentMatcher rather than making WindowRequestCleanupEvent a friend class and having it manipulate 'compartments' directly.

@@ +9169,5 @@
>  
> +class MultiCompartmentMatcher : public js::CompartmentFilter {
> +  friend class WindowRequestCleanupEvent;
> +
> +  js::HashSet<JSCompartment*,js::DefaultHasher<JSCompartment*>,js::SystemAllocPolicy> compartments;

mCompartments.

@@ +9187,5 @@
> +class PendingWindows {
> +  friend class WindowRequestCleanupEvent;
> +  friend class WindowDestroyedEvent;
> +  
> +  nsCOMArray<nsIWeakReference> windows;

mWindows.  Also please use nsTArray<nsWeakPtr>
Attachment #8693031 - Flags: review?(khuey) → review-
(In reply to Boris Zbarsky [:bz] (Vacation until Jan 4) from comment #8)
> Comment on attachment 8693031 [details] [diff] [review]
> fix_bug_816784.patch
> 
> Sorry for the lag here; I was trying to make sure I understood this code
> correctly.
> 
> So the biggest issue I see is that closing a single window does absolutely
> nothing to clean it up.  You have to close 9 other windows before anything
> gets cleaned up.  This seems fairly suboptimal for the normal browsing case.
> I would prefer it if we went ahead and did the cleanup after some time has
> passed since the last thing was enqueued to gPendingWindows, even if there
> aren't 10 windows in there yet.

Agreed.  It also needs to cleanup on memory-pressure.

> Apart from that:
> 
> 1)  Do we have any guarantees that the nsGlobalWindow can't die (nulling out
> the weakref) while its JSObject is still alive?  Especially in the outer
> window case, this worries me a bit.

The JSObject holds the inner window alive, no?  For the outer both will end up in the list, and an extant (un-cycle-collected) inner always holds alive the outer, so I think we're good here.

> 2)  Why do we want to add to gPendingWindows from WindowDestroyedEvent::Run,
> as opposed to from whatever posts the WindowDestroyedEvent?  Are we trying
> to ensure that the nuking comes after the NotifyObservers call there?  I
> guess we do need that; it's worth documenting.

Agreed that this should be documented.  Nuking definitely must happen *after* NotifyObservers.  I discovered that the hard way when I did this originally (IIRC nuking too early broke Jetpack).

> 3)  Having a statically allocated nsCOMArray is not OK.  It needs to be
> heap-allocated, and deleted late enough in shutdown.  Or better yet, just
> put the list of windows directly in the WindowRequestCleanupEvent and then
> keep a pointer to it around so you (1) can append to it and (2) don't bother
> posting a new one while one is live.

Agreed.

> 4)  During js::NukeCrossCompartmentWrappers, don't compartments possibly get
> destroyed as you go?  And if so, what makes sure that new ones don't get
> allocated in their spot, confusing the matcher?  Seems like you need to keep
> those compartments alive until after NukeCrossCompartmentWrappers returns.

js::NukeCrossCompartmentWrappers does not GC, so I don't think this is a concern.  All it does is replace relevant CCWs with a different proxy class (that throws on all accesses) and null out a slot.  See http://mxr.mozilla.org/mozilla-central/source/js/src/vm/ProxyObject.cpp#89
Sorry, but are there any news on when this bug will be resolved? It is planned for this quarter, or the next one?

For anyone who has lots of tabs it causes FireFox to few minutes to be closed, and sometimes it still can not finish all the nukes and crashes.

(Also, there is known bug that causes FireFox to become super slow after some active use, not only in closing tabs, but in just switching tabs. Some JS/GC issue, probably. The thing is that it is unresolved for years, and thus FireFox, being a "prosumer" browser, becomes barely usable.)

Thanks in advance.
Since upgrading to version 45, I got crash with new signature "shutdownhang | nsXPCWrappedJS::GetJSObject":

https://crash-stats.mozilla.com/report/index/129a2bec-d47a-4431-846d-994372160201



-- should it be filed as separate bug, or I should add second signature to #bug 1219586, whose signature is "[@ shutdownhang | js::NukeCrossCompartmentWrappers ]"?
Are you going to have a chance to address the review comments and upload a new version of the patch, Xuân Baldauf? If not, I can try to fix it up. Thanks.
Flags: needinfo?(baldauf--2015--bugzilla.mozilla.org)
Two new crash signatures:

1) "shutdownhang | kernelbase.dll@0x17ff":
https://crash-stats.mozilla.com/report/index/0fcb93bc-8dee-425b-b59d-a62c02160205

2) "shutdownhang | js::TraceEdge<T>":
https://crash-stats.mozilla.com/report/index/f80d6065-441f-46f1-bb24-7b4ea2160208
(In reply to User Dderss from comment #14)
> Two new crash signatures:

I think it would make sense to file a new bug for your issues. I don't see NukeCrossCompartmentWrappers in there anywhere, so it is probably a separate issue that is triggering the shutdown hang detector.
(In reply to Andrew McCreight [:mccr8] from comment #15)
 
> I think it would make sense to file a new bug for your issues. I don't see
> NukeCrossCompartmentWrappers in there anywhere, so it is probably a separate
> issue that is triggering the shutdown hang detector.

I have got another crash -- now FireFox crashes EVERY time when I try to close/restart it, it has yet another new signature "shutdownhang | CallTraceHook<T>": https://crash-stats.mozilla.com/report/index/c6b64d15-fe33-4212-8bde-606cd2160211

So I have last six shutdown crashes with different signature every time, but all are within toolkit/components/terminator/nsTerminator.cpp -- and mozilla::`anonymous namespace'::RunWatchdog(void*) function.

The route to crash is always this: when I close/restart FireFox, its memory footprint starts slowly increase (by as much as 500 MB or even more), not decrease, and it drags for several minutes until I get crash message.

My guess is that there is some timer on how long closing/terminating process can go, and since this process lasts forever, it just gets interrupted abruptly during whatever function that currently gets executed -- this is why I get random crash signatures, even though they all are in same nsTerminator.cpp and within the same RunWatchdog function.

This means that the root cause of this madness is still probably related to this bug. 

(I wonder when Mozilla will stop wasting resources of programmers on useless projects like messenger or FireFox OS and finally let them to fix FireFox. There are critical bugs like this one that do not get resolved for months or even years as there are not enough of engineers to fix them, and it is quite sad as in result FireFox loses its market share.)
(In reply to User Dderss from comment #16)
> This means that the root cause of this madness is still probably related to
> this bug. 

The "shutdownhang" just means that something was hanging during shut down. It looks like you are having long GCs for some reason. I'm not sure why that is, but it does not sound related to the issue in this bug, so please file a new issue. Putting unrelated information into this bug interferes with solving both issues. Thank you.

> (I wonder when Mozilla will stop wasting resources of programmers on useless
> projects like messenger or FireFox OS and finally let them to fix FireFox.

The amount of publicity a project gets is not necessarily related to the number of people working on it. I assure you that there are many people working on quality issues like these in Firefox. Unfortunately our resources are still finite, and not all issues can be fixed.
(In reply to Andrew McCreight [:mccr8] from comment #17)
> The "shutdownhang" just means that something was hanging during shut down.
> It looks like you are having long GCs for some reason. I'm not sure why that
> is, but it does not sound related to the issue in this bug, so please file a
> new issue. Putting unrelated information into this bug interferes with
> solving both issues. Thank you.

I did not know whether it is related or not, and this is why I asked; thanks for clearing this up.

However, I have five different shutdown crash signatures. Does this mean that I have to file five different bugs?
(In reply to User Dderss from comment #18)
> However, I have five different shutdown crash signatures. Does this mean
> that I have to file five different bugs?

One bug is fine, please. Hopefully they have the same root cause.
Crash Signature: [@ shutdownhang | js::UncheckedUnwrap ]
Whiteboard: [Snappy:?] → [Snappy:?][tw-dom]
I had only ~50 tabs open for this hang: https://crash-stats.mozilla.com/report/index/4601b837-dc89-4c75-85f4-2fdfc2160508#allthreads

I was, however, shutting down (command-Q) because Firefox had become sluggish, so there may have already been some pathological condition in play.
In release, the crash is spiking since 2016-05-06 (increased by ~158% until 2016-05-08) and is #45 in top-crashes for 46.0.1.
status-firefox46: --- → affected
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
Hi Andrew, I came upon this bug when reviewing top crashers on Beta47. This is ranked #26 but the thing that is peculiar about this one is that I see ~100 instances of this in 7 days on 46.0.1 and ~300 on Beta47. Based on the uptime distribution this does not seem to be a startup crash but is there a patch here that can be made ready soon? Perhaps something in time for us to gtb RC1. Thoughts?
Flags: needinfo?(continuation)
We should probably fix this, but this patch needs a bit of work, and it is a little too risky to land at this point in beta.
Flags: needinfo?(continuation)
Flags: needinfo?(baldauf--2015--bugzilla.mozilla.org)
Crash Signature: [@ shutdownhang | js::UncheckedUnwrap ] → [@ shutdownhang | js::UncheckedUnwrap ] [@ shutdownhang | js::NukeCrossCompartmentWrappers]
Blocks: 1344302
Blocks: 989816
Mentor: khuey
Whiteboard: [Snappy:?][tw-dom] → [qf:p1][Snappy:?][tw-dom]
Assignee: nobody → janus926
Status: NEW → ASSIGNED
If I understand attachment 8693031 [details] [diff] [review] correctly, it:

  1) delays the time we do NukeCrossCompartmentWrappers(),
  2) maybe make the time we spend in NukeCrossCompartmentWrappers() shorter because the patch walks through all the wrappers only once when there're more than one WindowDestroyedEvent, and we do this for every WindowDestroyedEvent now

But I am not sure how much 2) could help us, the time we spend will still be O(n) where n is the number of cross compartment wrappers. If it doesn't make the time a lot shorter, we will still see the hang sooner or later. Would nuking in iteration be reasonable?

There's one thing I am worried, what if some code accessing a "should be dead wrapper" during the delay that 1) creates, what will happen?
Flags: needinfo?(bzbarsky)
> But I am not sure how much 2) could help us

If I understood comments on related bugs correctly during startup lots of transient compartments are created and nuked, and since the nukeccw-code has to traverse all compartments for each compartment that is destroyed it results in O(n²) runtime during startup. Similar things happen when you close a window with multiple tabs. So there certainly would be some speedup happening, even if the code is still not optimal due to the lack of a reverse mapping for the wrappers.
I'm not sure sure why I'm being needinfo'd, but....

> But I am not sure how much 2) could help us, the time we spend will still
> be O(n) where n is the number of cross compartment wrappers.

Yes, but the question is what the constant is.  In comment 0, the constant is 1367 if we do the walk for every event, and 1 if we coalesce.  A 1000x speedup is not something to throw away lightly!

> Would nuking in iteration be reasonable?

I'm not sure what this is asking.

> what will happen?

You will get hold of things, then they will be replaced by dead wrappers.  Being a dead wrapper is already non-deterministic to a large extent, right?
Flags: needinfo?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #28)
> I'm not sure sure why I'm being needinfo'd, but....
> 
> > But I am not sure how much 2) could help us, the time we spend will still
> > be O(n) where n is the number of cross compartment wrappers.
> 
> Yes, but the question is what the constant is.  In comment 0, the constant
> is 1367 if we do the walk for every event, and 1 if we coalesce.  A 1000x
> speedup is not something to throw away lightly!

This example is extreme. The attachment 8693031 [details] [diff] [review] coalesces just 10 WindowDestroyedEvent, and comment 8 suggests to do NukeCrossCompartmentWrappers() after some time has passed, so it is still possible that 1 NukeCrossCompartmentWrappers() for 1 WindowDestroyedEvent.

> > Would nuking in iteration be reasonable?
> 
> I'm not sure what this is asking.
> 
> > what will happen?
> 
> You will get hold of things, then they will be replaced by dead wrappers. 
> Being a dead wrapper is already non-deterministic to a large extent, right?

If nuking CCW can be delayed, probably we don't need to nuke them all at once, but once a few source compartments.

But still, it'd be better if we can have a different data structure for storing the wrappers so we can find (search or index) them by their target compartment.
> so it is still possible that 1 NukeCrossCompartmentWrappers() for 1 WindowDestroyedEvent

Sure.  The point of this bug is to not do that when closing a bunch of tabs at once.
(In reply to Ting-Yu Chou [:ting] from comment #26)
>   2) maybe make the time we spend in NukeCrossCompartmentWrappers() shorter
> because the patch walks through all the wrappers only once when there're
> more than one WindowDestroyedEvent, and we do this for every
> WindowDestroyedEvent now
> 
> But I am not sure how much 2) could help us, the time we spend will still be
> O(n) where n is the number of cross compartment wrappers. If it doesn't make
> the time a lot shorter, we will still see the hang sooner or later. Would
> nuking in iteration be reasonable?

One thing I noticed when looking at these profiles is that a large chunk of the time spent nuking wrappers is in calling UncheckedUnwrap. So if we only have to do this once-per-wrapper when nuking 10 compartments, I suspect that even having to iterate over (up to) 10 compartments per wrapper when matching, it's still a significant savings.
I understand coalescing could relax the situation, just I'd like to also improve the time we spend in  NukeCrossCompartmentWrappers(), e.g., make WrapperMap a js::HashMap<JSCompartment*, NurseryAwareHashMap<CrossCompartmentKey, JS::Value, ...>>.
Does anyone have simpler STR instead of creating a thounsand tabs?
See Also: → 818296
(In reply to Ting-Yu Chou [:ting] from comment #34)
> Does anyone have simpler STR instead of creating a thounsand tabs?

I came across this running tp5o talos tests. For sites with a lot of frames, like cnn.com, it was taking about 35ms locally.
(In reply to Kris Maglione [:kmag] from comment #35)
> (In reply to Ting-Yu Chou [:ting] from comment #34)
> > Does anyone have simpler STR instead of creating a thounsand tabs?
> 
> I came across this running tp5o talos tests. For sites with a lot of frames,
> like cnn.com, it was taking about 35ms locally.

Another easy way that I have found to test this is to reload the FB home page.  On my fast computer the destroy event for the previous window of FB after the reload takes a few tens of ms easily: https://perfht.ml/2ofXGVG
(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #36)
> Another easy way that I have found to test this is to reload the FB home
> page.  On my fast computer the destroy event for the previous window of FB
> after the reload takes a few tens of ms easily: https://perfht.ml/2ofXGVG

Hm. This filter also includes time spent in JS processing the destroyed event. But it's actually even more relevant, since most of the time spent there is spent nuking content script sandboxes. That winds up being even more expensive than nuking web content windows, since we also scan all web content compartments in that case.

The CC should already be keeping track of which zones have edges into other zones... I wonder if it would help much to check if there are edges between the source and target compartments before iterating over the wrapper table.
Attached patch wip (obsolete) — — Splinter Review 
This uses 2 dimension hashmap (HashMap<JSCompartment*, NurseryAwareHashMap<>, ...>) to store the CCWs by target compartment, so we don't need to iterate through all CCWs to find nuking targets in NukeCrossCompartmentWrappers().

Here's the comparison of the time we spend in the for loop of NukeCrossCompartmentWrappers() without/with the wip when reload Facebook (I reloaded it 10 times, each line shows the timestamps before/after the for loop and rounded diff):

Without:

  171420.446535514 171420.446748803 213us
  171423.359798130 171423.360123287 325us
  171425.681580144 171425.681897242 317us
  171427.452394870 171427.452694012 299us
  171429.352813970 171429.353144477 331us
  171431.358817752 171431.359168519 351us
  171433.355926667 171433.356351208 425us
  171435.373670815 171435.373986148 315us
  171437.319469451 171437.319809914 340us
  171442.063216062 171442.063677043 461us
  MAX=461 MIN=213 MED=328 AVG=337.7 STD=67.663

With:

  171726.823876235 171726.823957140 81us
  171728.760239632 171728.760349238 110us
  171730.708486263 171730.708601678 115us
  171732.591743405 171732.591876376 133us
  171734.601805171 171734.601957624 152us
  171736.633955058 171736.634041303 86us
  171738.354415408 171738.354527997 113us
  171739.774965966 171739.775110000 144us
  171741.103018624 171741.103158625 140us
  171742.500812435 171742.500961187 149us
  MAX=149 MIN=81 MED=124 AVG=122.3 STD=25.404

Note string wrappers are stored separately because it has target compartment nullptr, which now we can iterate through only object wrappers.
Attachment #8860251 - Attachment is patch: true
I'm checking why the WIP makes e.front().key().compartment() unmatched to wrapped->compartment() in NukeCrossCompartmentWrappers() sometimes.
Is |k.wrapped| always equals to |wrapped| in NukeCrossCompartmentWrappers() [1]? If yes, why do we need to UncheckedUnwrap() in the for loop? If not, when will they be different?

[1] http://searchfox.org/mozilla-central/source/js/src/proxy/CrossCompartmentWrapper.cpp#545
Flags: needinfo?(terrence.d.cole)
Flags: needinfo?(terrence.d.cole) → needinfo?(sphink)
(In reply to Ting-Yu Chou [:ting] from comment #39)
> I'm checking why the WIP makes e.front().key().compartment() unmatched to
> wrapped->compartment() in NukeCrossCompartmentWrappers() sometimes.

Turns out it's not because of the WIP, it's the key/value pair pass to js::WrapperMap::put() sometimes have unmatched compartment between k.compartment() and UncheckedUnwrap(v.toObjectOrNull())->compartment().

Is this expected?
(In reply to Ting-Yu Chou [:ting] from comment #40)
> Is |k.wrapped| always equals to |wrapped| in NukeCrossCompartmentWrappers()
> [1]? If yes, why do we need to UncheckedUnwrap() in the for loop? If not,
> when will they be different?

So the answer is no, they're not always equal.

:sfink, it'd be great if you could shed me light on when/why they're different and the question in comment 41.
I think the problem is that you're not moving the wrapper to the list for the new compartment after brain transplants (unless I'm missing something), so when a window navigates, window proxies that pointed to the old inner window will be in the wrong list.
The questions I raised were the behavior of m-c, so the problem is not about the wip. I raised them because I thought the |wrapped| of CrossCompartmentKey always equals to the unwrapped value (also their compartment), but I found it does not. I wonder why and when could this happen, so I can decide how to treat them differently when I store CCWs in a 2d hashmap.

The brain transplants you mentioned sounds like js::RemapWrapper() which looks up the old wrapper, removes it, rewrap and puts the new wrapper, if it is, it will maintain the correctness.
(In reply to Ting-Yu Chou [:ting] from comment #44)
> The questions I raised were the behavior of m-c, so the problem is not about
> the wip. I raised them because I thought the |wrapped| of
> CrossCompartmentKey always equals to the unwrapped value (also their
> compartment), but I found it does not. I wonder why and when could this
> happen, so I can decide how to treat them differently when I store CCWs in a
> 2d hashmap.

Sorry, I guess I misunderstood. I think the answer is still window proxies, since UncheckedUnwrap doesn't unwrap them by default. But in that case, I'm not sure.

> The brain transplants you mentioned sounds like js::RemapWrapper() which
> looks up the old wrapper, removes it, rewrap and puts the new wrapper, if it
> is, it will maintain the correctness.

Ah, good. I thought we reused the old wrapper map entry.
I found the CCW that I have trouble with is from Debugger::wrapDebuggeeObject():

  http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/js/src/vm/Debugger.cpp#1230

with the stack:

  #1  0x00007fffe9f31fbc in js::WrapperMap::put (this=0x7fffbb9d6950, 
      k=..., v=...) at /home/ting/w/fx/mc/js/src/jscompartment.h:410
  #2  0x00007fffe9f125ea in JSCompartment::putWrapper (
      this=0x7fffbb9d6800, cx=0x7fffdfd0c800, wrapped=..., 
      wrapper=...) at /home/ting/w/fx/mc/js/src/jscompartment.cpp:271
  #3  0x00007fffea0cabce in js::Debugger::wrapDebuggeeObject (
      this=0x7fffc0d39000, cx=0x7fffdfd0c800, obj=..., result=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:1230
  #4  0x00007fffea0ca510 in js::Debugger::wrapDebuggeeValue (
      this=0x7fffc0d39000, cx=0x7fffdfd0c800, vp=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:1164
  #5  0x00007fffea0cf041 in js::Debugger::fireNewGlobalObject (
      this=0x7fffc0d39000, cx=0x7fffdfd0c800, global=..., vp=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:2151
  #6  0x00007fffea0cf814 in js::Debugger::slowPathOnNewGlobalObject (
      cx=0x7fffdfd0c800, global=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.cpp:2222
  #7  0x00007fffe9f32451 in js::Debugger::onNewGlobalObject (
      cx=0x7fffdfd0c800, global=...)
      at /home/ting/w/fx/mc/js/src/vm/Debugger.h:1770

It new a DebuggerObject |dobj| and passes it as the wrapper to putWrapper(), which has different compartment with the referent object it sets to CrossCompartmentKey. When the DebuggerObject goes to NukeCrossCompartmentWrapper() in the WIP, it crashed at:

  http://searchfox.org/mozilla-central/rev/ce5ccb6a8ca803271c946ccb8b43b7e7d8b64e7a/js/src/vm/ProxyObject.cpp#117

because DebuggerObject is neither a ProxyObject, nor a WrapperObject, nor a CrossCompartmentWrapperObject and handler() returns an invalid pointer.

:jimb, are those CCWs put in Debugger.cpp expected not to be nuked?
Flags: needinfo?(sphink) → needinfo?(jimb)
We unfortunately don't nuke debugger references (see bug 1084626), which causes us to leak pages when using things like the browser console.
Thanks for that information, :mccr8. I'll skip nuking those debugger references here.
Flags: needinfo?(jimb)
The only thing remained that I want to clarify is can we skip UncheckedUnwrap() and use the |wrapped| in CrossCompartmentKey directly if we neglect those debugger references.
There're still chances that UncheckedUnwrap() returns a different JSObject* from the |wrapped| of CrossCompartmentKey, but in most cases they're the same and UncheckedUnwrap() unwrap just once (from a wrapper to its wrapped). If |wrapped| is somewhere on the unwrapping chain, passing it to UncheckedUnwrap() instead of the wrapper object will safe us a bit of time.

Try [1] looks good, I'm going to polish the patch and send to review.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=2094508514ce5275e7a9f3d26ccd0aa5cda9cdba&group_state=expanded
See Also: → 1361461
Blocks: 1360681
Am I the best person to review nuking? I feel maybe 60% comfortable.
Then do you know whom should I ask for reviewing?
Maybe :jonco could review? He's been looking at NukeCrossCompartmentWrappers a fair amount recently.Richard Barnes [:rbarnes]
Jon, do you feel comfortable stealing these nuking reviews from me?
Flags: needinfo?(jcoppeard)
Comment on attachment 8864728 [details]
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.

https://reviewboard.mozilla.org/r/136372/#review142514

Thanks for the patch.  The approach seems fine but I do have a few comments.

::: js/src/jscompartment.h:188
(Diff revision 1)
>          } matcher(f);
>          return wrapped.match(matcher);
>      }
>  
>      // Valid for JSObject* and Debugger keys. Crashes immediately if used on a
>      // JSString* key.

Please remove the comment about string keys here if we're going to change this.

::: js/src/jscompartment.h:240
(Diff revision 1)
>    private:
>      CrossCompartmentKey() = delete;
>      WrappedType wrapped;
>  };
>  
> +class WrapperMap

Please add a comment explaining that there is a map per compartment, and that strings are stored separately.

::: js/src/jscompartment.h:256
(Diff revision 1)
> +    OuterMap map;
> +
> +  public:
> +    class Enum
> +    {
> +        using InnerMapEntry = decltype(std::declval<InnerMap::Ptr>().operator*());

Is this not InnerMap::Entry?

::: js/src/jscompartment.h:285
(Diff revision 1)
> +                outer.popFront();
> +            }
> +        }
> +
> +        OuterMap::Enum outer;
> +        InnerMap::Enum* inner;

Please use mozilla::Maybe<InnerMap::Enum> here.  js_new is fallible and it would be better not to repeatedly allocate and free the memory.

::: js/src/jscompartment.h:292
(Diff revision 1)
> +      public:
> +        Enum(WrapperMap& m) : outer(m.map), inner(nullptr) {
> +            goToNext();
> +        }
> +
> +        Enum(WrapperMap& m, const SingleCompartment& f) : outer(dummy()),

The dummy() map is pretty strange and I don't think its initialisation it's threadsafe.  Can you use Maybe<> for the outer Enum too to handle this?

::: js/src/jscompartment.h:364
(Diff revision 1)
> +            p.map->remove(p);
> +        }
> +    }
> +
> +    MOZ_MUST_USE bool put(const CrossCompartmentKey& k, const JS::Value& v) {
> +        JSCompartment* c = const_cast<CrossCompartmentKey&>(k).compartment();

Please add an assert that compartment is null iff the key is a string to help explain what's going on here.

::: js/src/jscompartment.h:368
(Diff revision 1)
> +    MOZ_MUST_USE bool put(const CrossCompartmentKey& k, const JS::Value& v) {
> +        JSCompartment* c = const_cast<CrossCompartmentKey&>(k).compartment();
> +        auto p = map.lookupForAdd(c);
> +        if (!p) {
> +            InnerMap m;
> +            if (!m.init(0) || !map.add(p, c, mozilla::Move(m))) {

Passing an initial length of zero is strange when we're about to add something.  You could leave this at the default or specify something small if you think that would be better.

::: js/src/jscompartment.h:842
(Diff revision 1)
>          crossCompartmentWrappers.remove(p);
>      }
>  
>      struct WrapperEnum : public js::WrapperMap::Enum {
>          explicit WrapperEnum(JSCompartment* c) : js::WrapperMap::Enum(c->crossCompartmentWrappers) {}
> +        explicit WrapperEnum(JSCompartment* c, const js::SingleCompartment& f) : js::WrapperMap::Enum(c->crossCompartmentWrappers, f) {}

I'm not sure we need the compartment filter here - just passing |JSCompartment* target| would be fine.
Attachment #8864728 - Flags: review?(jcoppeard)
Comment on attachment 8864729 [details]
Bug 816784 part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers.

https://reviewboard.mozilla.org/r/136374/#review142520

::: js/src/jsfriendapi.h:1207
(Diff revision 1)
>  };
>  
>  extern JS_FRIEND_API(bool)
>  NukeCrossCompartmentWrappers(JSContext* cx,
>                               const CompartmentFilter& sourceFilter,
> -                             const CompartmentFilter& targetFilter,
> +                             const SingleCompartment& targetFilter,

There's not much point passing a filter if it's always got to be the same kind of filter.  We could just take a JSCompartment* here.

::: js/src/proxy/CrossCompartmentWrapper.cpp:542
(Diff revision 1)
> -        for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
> -            // Some cross-compartment wrappers are for strings.  We're not
> -            // interested in those.
> +        // |nukeAll| is true. The wrappers for strings that we're not interested
> +        // in won't be here because they have compartment nullptr.
> +        const JSCompartment::WrapperEnum& ce = MOZ_LIKELY(!nukeAll) ?
> +                JSCompartment::WrapperEnum(c, targetFilter) :
> +                JSCompartment::WrapperEnum(c);
> +        JSCompartment::WrapperEnum& e = const_cast<JSCompartment::WrapperEnum&>(ce);

Why the const cast?  Can we remove the const from the definition of ce?

::: js/src/proxy/CrossCompartmentWrapper.cpp:555
(Diff revision 1)
>              AutoWrapperRooter wobj(cx, WrapperValue(e));
> -            JSObject* wrapped = UncheckedUnwrap(wobj);
>  
> +            // Unwrap from the wrapped object in CrossCompartmentKey instead of
> +            // the wrapper, this could save us a bit of time.
> +            JSObject* wrapped = UncheckedUnwrap(k.as<JSObject*>());

I don't get why this is faster.
Attachment #8864729 - Flags: review?(jcoppeard)
Comment on attachment 8864730 [details]
Bug 816784 part 3 - Optimize the other places that iterate CCWs.

https://reviewboard.mozilla.org/r/136376/#review142522

::: js/src/jscompartment.h:294
(Diff revision 2)
>              }
>          }
>  
>          OuterMap::Enum outer;
>          InnerMap::Enum* inner;
> +        CompartmentFilter* filter;

Please make this const and lose the cast below.

::: js/src/jscompartment.h:298
(Diff revision 2)
>          InnerMap::Enum* inner;
> +        CompartmentFilter* filter;
> +        bool skipString;
>  
>        public:
> -        Enum(WrapperMap& m) : outer(m.map), inner(nullptr) {
> +        Enum(WrapperMap& m, bool skipString = false) : outer(m.map),

Can you add an enum for this so it's clearer in the caller?  e.g. WithStrings/WithoutStrings.

::: js/src/jscompartment.h:865
(Diff revision 2)
>      };
>  
> +    struct NonStrWrapperEnum : public js::WrapperMap::Enum {
> +        explicit NonStrWrapperEnum(JSCompartment* c) : js::WrapperMap::Enum(c->crossCompartmentWrappers, true) {}
> +        explicit NonStrWrapperEnum(JSCompartment* c, const js::CompartmentFilter& f) : js::WrapperMap::Enum(c->crossCompartmentWrappers, f, true) {}
> +    };

Please call this NonStringWrapperEnum and assert that the target compartment is not null in the second constructor.

::: js/src/jsgc.cpp:4416
(Diff revision 2)
>       */
> +    const SingleCompartment& f = js::SingleCompartment(nullptr);
>      for (CompartmentsIter c(rt, SkipAtoms); !c.done(); c.next()) {
> -        for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
> +        for (JSCompartment::WrapperEnum e(c, f); !e.empty(); e.popFront()) {
> -            if (e.front().key().is<JSString*>())
> -                e.removeFront();
> +            e.removeFront();

Please add an assertion that the wrappers found by this are all string wrappers.
Flags: needinfo?(jcoppeard)
Attachment #8860251 - Attachment is obsolete: true
Comment on attachment 8864728 [details]
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.

https://reviewboard.mozilla.org/r/136372/#review142514

> Please remove the comment about string keys here if we're going to change this.

Removed.

> Please add a comment explaining that there is a map per compartment, and that strings are stored separately.

Added.

> Is this not InnerMap::Entry?

Added the type "Entry" to NurseryAwareHashMap and removed this line.

> Please use mozilla::Maybe<InnerMap::Enum> here.  js_new is fallible and it would be better not to repeatedly allocate and free the memory.

I wasn't aware of Maybe, that's a lot better now, thanks.

> The dummy() map is pretty strange and I don't think its initialisation it's threadsafe.  Can you use Maybe<> for the outer Enum too to handle this?

Fixed.

> Please add an assert that compartment is null iff the key is a string to help explain what's going on here.

Added.

> Passing an initial length of zero is strange when we're about to add something.  You could leave this at the default or specify something small if you think that would be better.

Fixed.

> I'm not sure we need the compartment filter here - just passing |JSCompartment* target| would be fine.

Fixed.
Comment on attachment 8864730 [details]
Bug 816784 part 3 - Optimize the other places that iterate CCWs.

https://reviewboard.mozilla.org/r/136376/#review142522

> Please make this const and lose the cast below.

Fixed.

> Can you add an enum for this so it's clearer in the caller?  e.g. WithStrings/WithoutStrings.

Added.

> Please call this NonStringWrapperEnum and assert that the target compartment is not null in the second constructor.

Fixed the name, but couldn't add the assertion because not every filter has a pointer to the target compartment.

> Please add an assertion that the wrappers found by this are all string wrappers.

Added.
Comment on attachment 8864728 [details]
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.

https://reviewboard.mozilla.org/r/136372/#review142538

::: js/src/jscompartment.h:238
(Diff revisions 1 - 2)
>    private:
>      CrossCompartmentKey() = delete;
>      WrappedType wrapped;
>  };
>  
> +// The data structure for storing CCWs, which there's a map per target

s/there's/has

::: js/src/jscompartment.h:263
(Diff revisions 1 - 2)
>          void operator=(const Enum&) = delete;
>  
>          void goToNext() {
> -            while (!outer.empty()) {
> -                InnerMap& m = outer.front().value();
> +            if (outer.isNothing()) {
> +                return;
> +            }

nit: JS engine style is not to use braces if the body of the if is only one line.

::: js/src/jscompartment.h:269
(Diff revisions 1 - 2)
> +            while (!outer->empty()) {
> +                InnerMap& m = outer->front().value();
>                  if (!m.empty()) {
> -                    if (inner) {
> -                        js_delete(inner);
> +                    if (inner.isSome()) {
> +                        inner.reset();
>                      }

Ditto above.

::: js/src/jscompartment.h:289
(Diff revisions 1 - 2)
>              goToNext();
>          }
>  
> -        Enum(WrapperMap& m, const SingleCompartment& f) : outer(dummy()),
> -                                                          inner(nullptr) {
> -            // The outer map is empty, will iterate only the inner map we found
> +        Enum(WrapperMap& m, JSCompartment* target) {
> +            // The outer map is nothing, will iterate only the inner map we
> +            // found here.

How about "Leave the outer map as nothing and only iterate the inner map we find here"?

::: js/src/jscompartment.h:293
(Diff revisions 1 - 2)
> -        }
> -
> -        ~Enum() {
> -            if (inner) {
> -                js_delete(inner);
>              }

Please remove braces from single-line if statement.

::: js/src/jscompartment.h:354
(Diff revisions 1 - 2)
>          }
>      }
>  
>      MOZ_MUST_USE bool put(const CrossCompartmentKey& k, const JS::Value& v) {
>          JSCompartment* c = const_cast<CrossCompartmentKey&>(k).compartment();
> +        MOZ_ASSERT_IF(k.is<JSString*>(), !c);

I think we can make this stronger and assert |k.is<JSString*> == !c|.

::: js/src/jscompartment.h:358
(Diff revisions 1 - 2)
>          JSCompartment* c = const_cast<CrossCompartmentKey&>(k).compartment();
> +        MOZ_ASSERT_IF(k.is<JSString*>(), !c);
>          auto p = map.lookupForAdd(c);
>          if (!p) {
>              InnerMap m;
> -            if (!m.init(0) || !map.add(p, c, mozilla::Move(m))) {
> +            if (!m.init(4) || !map.add(p, c, mozilla::Move(m))) {

Please make this a constant.
Attachment #8864728 - Flags: review?(jcoppeard)
Comment on attachment 8864729 [details]
Bug 816784 part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers.

https://reviewboard.mozilla.org/r/136374/#review142520

> There's not much point passing a filter if it's always got to be the same kind of filter.  We could just take a JSCompartment* here.

Fixed.

> Why the const cast?  Can we remove the const from the definition of ce?

I did so because the temporary object of the ternary operator will bound to the const reference ce, which I expect to prevent a copy.

> I don't get why this is faster.

It's faster because then in most cases UncheckedUnwrap() won't unwrap anything and will return earlier because of the check here: http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/js/src/proxy/Wrapper.cpp#344
Comment on attachment 8864728 [details]
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.

https://reviewboard.mozilla.org/r/136372/#review142538

> s/there's/has

Fixed.

> nit: JS engine style is not to use braces if the body of the if is only one line.

Fixed.

> How about "Leave the outer map as nothing and only iterate the inner map we find here"?

Fixed.

> Please remove braces from single-line if statement.

Removed.

> I think we can make this stronger and assert |k.is<JSString*> == !c|.

Fixed.

> Please make this a constant.

Fixed.
(In reply to Ting-Yu Chou [:ting] from comment #68)
> > Why the const cast?  Can we remove the const from the definition of ce?
> 
> I did so because the temporary object of the ternary operator will bound to
> the const reference ce, which I expect to prevent a copy.

Ideally we'd just have a WrapperEnum here rather than a reference but that doesn't work because at the moment because there's no viable move constructor for HashTable::Enum (or WrapperMap::Enum).

I've filed a bug 1365654 for this.

> > I don't get why this is faster.
> 
> It's faster because then in most cases UncheckedUnwrap() won't unwrap
> anything 

Oh right, I see now.
I fixed the static analysis error, and replaced the const reference by a Maybe.

The reason I used the const reference was to avoid copying from conditionally initializing WrapperEnum, I did some experiments and found both UniquePtr and Maybe can have the same results also the compilers are happy.
(In reply to Ting-Yu Chou [:ting] from comment #80)
Great.  Thanks for all the updates.
Comment on attachment 8864728 [details]
Bug 816784 part 1 - Use a 2d hashmap to store cross compartment wrappers.

https://reviewboard.mozilla.org/r/136372/#review144078
Attachment #8864728 - Flags: review?(jcoppeard) → review+
Comment on attachment 8864729 [details]
Bug 816784 part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers.

https://reviewboard.mozilla.org/r/136374/#review144086

::: js/src/proxy/CrossCompartmentWrapper.cpp:541
(Diff revision 5)
> -        // Iterate the wrappers looking for anything interesting.
> -        for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
> -            // Some cross-compartment wrappers are for strings.  We're not
> -            // interested in those.
> -            const CrossCompartmentKey& k = e.front().key();
> +        // Iterate only the wrappers that have target compartment matched unless
> +        // |nukeAll| is true. The wrappers for strings that we're not interested
> +        // in won't be here because they have compartment nullptr. Use Maybe to
> +        // avoid copying from conditionally initializing WrapperEnum.
> +        mozilla::Maybe<JSCompartment::WrapperEnum> e;
> +        MOZ_LIKELY(!nukeAll) ? e.emplace(c, target) : e.emplace(c);

Please do this with an if statement if you're not using the result of the ternary expression.
Attachment #8864729 - Flags: review?(jcoppeard) → review+
Comment on attachment 8864730 [details]
Bug 816784 part 3 - Optimize the other places that iterate CCWs.

https://reviewboard.mozilla.org/r/136376/#review142536

::: js/src/jscompartment.h:258
(Diff revision 3)
>  
>    public:
>      class Enum
>      {
> +      public:
> +        enum Inclusion : bool {

Please call this SkipStrings or similar so that the name corresponds to the true/false values.

::: js/src/jscompartment.h:299
(Diff revision 3)
>          mozilla::Maybe<InnerMap::Enum> inner;
> +        const CompartmentFilter* filter;
> +        Inclusion skipStrings;
>  
>        public:
> -        Enum(WrapperMap& m) {
> +        Enum(WrapperMap& m, Inclusion i = WithStrings) : filter(nullptr),

nit: member initialisers should start on the next line.

::: js/src/jscompartment.cpp:684
(Diff revision 3)
>  JSCompartment::traceOutgoingCrossCompartmentWrappers(JSTracer* trc)
>  {
>      MOZ_ASSERT(JS::CurrentThreadIsHeapMajorCollecting());
>      MOZ_ASSERT(!zone()->isCollectingFromAnyThread() || trc->runtime()->gc.isHeapCompacting());
>  
> -    for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +    for (WrapperMap::Enum e(crossCompartmentWrappers, WrapperMap::Enum::WithoutStrings);

I guess we should use NonStringWrapperEnum here.

::: js/src/jsgc.cpp:4414
(Diff revision 3)
>       * String "wrappers" are dropped on GC because their presence would require
>       * us to sweep the wrappers in all compartments every time we sweep a
>       * compartment group.
>       */
>      for (CompartmentsIter c(rt, SkipAtoms); !c.done(); c.next()) {
> -        for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
> +        for (JSCompartment::WrapperEnum e(c, nullptr); !e.empty(); e.popFront()) {

Passing nullptr for the compartment here makes for a strange API.  Can you add a StringWrapperEnum for this case?

I think this means you can remove the second WrapperEnum constructor that takes a target compartment too.
Comment on attachment 8864729 [details]
Bug 816784 part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers.

https://reviewboard.mozilla.org/r/136374/#review144086

> Please do this with an if statement if you're not using the result of the ternary expression.

Fixed.
Comment on attachment 8864730 [details]
Bug 816784 part 3 - Optimize the other places that iterate CCWs.

https://reviewboard.mozilla.org/r/136376/#review142536

> Please call this SkipStrings or similar so that the name corresponds to the true/false values.

Fixed.

> nit: member initialisers should start on the next line.

Fixed.

> I guess we should use NonStringWrapperEnum here.

Fixed.

> Passing nullptr for the compartment here makes for a strange API.  Can you add a StringWrapperEnum for this case?
> 
> I think this means you can remove the second WrapperEnum constructor that takes a target compartment too.

Added StringWrapperEnum. For the 2nd WrapperEnum constructor, I just realized that I forgot to change to NonStringWrapperEnum in NukeCrossCompartmentWrappers(), moved the 2nd constructor to NonStringWrapperEnum and fixed NukeCrossCompartmentWrappers().
Comment on attachment 8864730 [details]
Bug 816784 part 3 - Optimize the other places that iterate CCWs.

https://reviewboard.mozilla.org/r/136376/#review144562

Great.  Thanks for making all the updates.

::: js/src/jsgc.cpp:4707
(Diff revision 7)
>      for (CompartmentsIter c(rt, SkipAtoms); !c.done(); c.next()) {
>          MOZ_ASSERT(!c->gcIncomingGrayPointers);
> -        for (JSCompartment::WrapperEnum e(c); !e.empty(); e.popFront()) {
> +        for (JSCompartment::NonStringWrapperEnum e(c); !e.empty(); e.popFront()) {
> -            if (!e.front().key().is<JSString*>())
> -                AssertNotOnGrayList(&e.front().value().unbarrieredGet().toObject());
> +            AssertNotOnGrayList(&e.front().value().unbarrieredGet().toObject());
>          }

nit: braces not required on if the body is a single line.
Attachment #8864730 - Flags: review?(jcoppeard) → review+
Try [1] failed at hazard-linux64-haz/debug, the stack includes WrapperMap::Enum but it looks odd because InCrossCompartmentMap() never triggers BrowserCompartmentMatcher::match():

GC Function: _Z26JS_GetTypedArraySharednessP8JSObject$uint8 JS_GetTypedArraySharedness(JSObject*)
    JSObject* js::CheckedUnwrap(JSObject*, uint8)
    JSObject* js::UnwrapOneChecked(JSObject*, uint8)
    JSObject* js::Wrapper::wrappedObject(JSObject*)
    GCAPI.h:void JS::ExposeObjectToActiveJS(JSObject*)
    GCAPI.h:void js::gc::ExposeGCThingToActiveJS(JS::GCCellPtr)
    uint8 JS::UnmarkGrayGCThingRecursively(JS::GCCellPtr)
    uint8 js::UnmarkGrayCellRecursively(js::gc::Cell*, int32)
    uint8 (UnmarkGrayCellRecursivelyFunctor, void*, int32), (Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) JS::DispatchTraceKindTyped(F, void*, JS::TraceKind, Args&& ...) [with F = UnmarkGrayCellRecursivelyFunctor; Args = {}; decltype (f(static_cast<JSObject*>(nullptr), (Forward<Args>)(JS::DispatchTraceKindTyped::args)...)) = bool]
    uint8 UnmarkGrayCellRecursivelyFunctor::operator(js::Shape*)(T*) [with T = js::Shape]
    Marking.cpp:uint8 TypedUnmarkGrayCellRecursively(js::Shape*) [with T = js::Shape]
    void UnmarkGrayTracer::unmark(JS::GCCellPtr)
    void CompartmentCheckTracer::onChild(JS::GCCellPtr*)
    jsgc.cpp:uint8 InCrossCompartmentMap(JSObject*, JS::GCCellPtr)
    void js::WrapperMap::Enum::popFront()
    void js::WrapperMap::Enum::goToNext()
    uint8 BrowserCompartmentMatcher::match(JSCompartment*) const
    uint8 nsContentUtils::IsSystemOrExpandedPrincipal(nsIPrincipal*)
    uint8 nsContentUtils::IsExpandedPrincipal(nsIPrincipal*)
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsIExpandedPrincipal]
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsIExpandedPrincipal]
    void nsCOMPtr<T>::assign_from_qi(nsQueryInterface, const nsIID&) [with T = nsIExpandedPrincipal; nsIID = nsID]
    uint32 nsQueryInterface::operator(nsID*, void**)(const nsIID&, void**) const
    FieldCall: nsISupports.QueryInterface

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=65a614f837e933b7b56c3ef129389bb89a9b3ab1
Maybe sfink has some ideas about you hazard failures.
Flags: needinfo?(sphink)
Based on the wiki [1], I probably should ask :jonco for the rooting hazards.

:jonco, do you have any ideas how could the stack be possible and how should I debug this? I tried to run browser analysis locally, but unfortunately I use Ubuntu...

[1] https://wiki.mozilla.org/Javascript:Hazard_Builds#Diagnosing_a_rooting_hazards_failure
Flags: needinfo?(sphink) → needinfo?(jcoppeard)
I kind of understand what is this about now.

So WrapperMap::Enum::goToNext() is listed as a GC function because of the stack below. But it seems that the stack is *generated* because BrowserCompartmentMatcher is a subclass of CompartmentFilter, even though BrowserCompartmentMatcher is never the filter of WrapperMap::Enum. And then all the places calling Wrapper::Enum::popFront() which goes to goToNext() will be reported as a rooting hazard if it has unrooted GC things.

GC Function: _ZN2js10WrapperMap4Enum8goToNextEv$void js::WrapperMap::Enum::goToNext()
    uint8 BrowserCompartmentMatcher::match(JSCompartment*) const
    uint8 nsContentUtils::IsSystemOrExpandedPrincipal(nsIPrincipal*)
    uint8 nsContentUtils::IsExpandedPrincipal(nsIPrincipal*)
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsIExpandedPrincipal]
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsIExpandedPrincipal]
    void nsCOMPtr<T>::assign_from_qi(nsQueryInterface, const nsIID&) [with T = nsIExpandedPrincipal; nsIID = nsID]
    uint32 nsQueryInterface::operator(nsID*, void**)(const nsIID&, void**) const
    FieldCall: nsISupports.QueryInterface
Is adding WrapperMap::Enum::goToNext() to ignoreFunctions[] in annotations.js a good way to fix this?
Sorry, I should have noticed this bug sooner. I'm looking at it now. The annotation you suggested would work, but I want to see if there's something more precise we could use here.
Flags: needinfo?(jcoppeard) → needinfo?(sphink)
Can you try adding 
    uint8 nsContentUtils::IsExpandedPrincipal(nsIPrincipal*)
to ignoreFunctions[] first?

I'd like to figure out how I can preserve type information when going through
    uint32 nsQueryInterface::operator(nsID*, void**)(const nsIID&, void**) const
because then no annotation would be necessary. But in the meantime, IsExpandedPrincipal is more narrowly scoped (and therefore less likely to hide problems).

It may not work, if there's another path to a FieldCall. In that case, go ahead and add the goToNext annotation and file a bug for fixing IsExpandedPrincipal.
Flags: needinfo?(sphink)
Never mind about filing the bug. I filed bug 1367132 for this, though I'm doubtful that approach will work. (I can probably hack something specific into the analysis to do it differently, though.)
(In reply to Steve Fink [:sfink] [:s:] from comment #97)
> Can you try adding 
>     uint8 nsContentUtils::IsExpandedPrincipal(nsIPrincipal*)
> to ignoreFunctions[] first?

It works.
Comment on attachment 8870675 [details]
Bug 816784 part 4 - Fix the rooting hazards that the analysis report.

https://reviewboard.mozilla.org/r/142142/#review145982
Attachment #8870675 - Flags: review?(sphink) → review+
Pushed by tchou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7e8f428a3edf
part 1 - Use a 2d hashmap to store cross compartment wrappers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/e80197b11567
part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/ac4a48a831ce
part 3 - Optimize the other places that iterate CCWs. r=jonco
https://hg.mozilla.org/integration/autoland/rev/9a553b9dc922
part 4 - Fix the rooting hazards that the analysis report. r=sfink
It's odd that those crashes didn't appear in previous Try [1], and from the stack I can't see any thing related to the patches. I'll try to rerun them.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e19929460958874b75997bf79dd0b653304bd01f&group_state=expanded
There's been a LOT of GC crashes in automation lately with morphing signatures, so it's entirely possible you got caught up in the fray :(. Sorry, hopefully they'll get sorted out soon.
If that's the case, under what requirements I can reland, maybe if the reruns are green?
The reruns (8 times) are all green...
(In reply to Ryan VanderMeulen [:RyanVM] from comment #103)
> https://treeherder.mozilla.org/logviewer.html#?job_id=101590721&repo=autoland
I didn't see any other crashes like this after the backout. However, I wasn't able to reproduce them on Try after multiple retriggers either, though.
> https://treeherder.mozilla.org/logviewer.html#?job_id=101600588&repo=autoland
This wasn't your fault.

Anyway, I can't see anything obviously wrong with that Try push, so I'll chalk it up to a false alarm and re-land (keeping an eye out for any issues should they reappear). Sorry for the hassle :(. Here's to hoping the GC crashes in CI get under control soon!
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/221302eab0e9
part 1 - Use a 2d hashmap to store cross compartment wrappers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/68d23dbb4af0
part 2 - Optimize NukeCrossCompartmentWrappers() to iterate only the targetting wrappers. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8f5611b2e350
part 3 - Optimize the other places that iterate CCWs. r=jonco
https://hg.mozilla.org/integration/autoland/rev/dc8dd5603ef0
part 4 - Fix the rooting hazards that the analysis report. r=sfink
No needs to sorry, I understand why and thank you for doing this. :)
Component: DOM → DOM: Core & HTML
Performance Impact: --- → P1
Whiteboard: [qf:p1][Snappy:?][tw-dom] → [Snappy:?][tw-dom]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: