1141614 - [marker] trace cycle collection with timeline markers

Status: RESOLVED FIXED

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

Description

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Similar to bug 1137844, but for cycle collection rather than garbage collection.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

So it looks like the place to gather this data and then post a runnable to fire this hook for observing debuggers would be in `nsJSContext::EndCycleCollectionCallback`.

I'm under the impression that CC is also incremental, but it isn't clear to me if this callback is called at the end of each CC slice or at the end of a CC cycle. I think the former? If so, then it will be more difficult to implement the same per-cycle API we have for onGarbageCollection. However, some Debugger API users have requested that onGarbageCollection switch to firing per-slice, so maybe it makes sense to fire this hook per-slice and then update the onGarbageCollection hook to be per-slice as well in the future.

We don't seem to have reasons for CC, like we do for GC.

CycleCollectorStats uses Timestamp, which is great as that's what the SPS profiler and markers use. Soon the onGarbageCollection hook will use it too: bug 1159506.

Comment 2

[Andrew McCreight [:mccr8]]

The two CC callbacks in nsJSContext are called at the very start and the very end of the cycle collection.

Comment 3

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

What information does the debugger need? We have the observer notifications already, it is fired
in EndCycleCollectionCallback. Can we reuse that?
Or does debugger want to get called whenever we start/end a CC slice? If so, I think also forgetSkippable slices should be considered.

(forgetSkippable slices run before the actual incremental CC. forgetSkippable is about optimizing out certainly alive objects from the CC graph. It needs several slices since part of the optimization is black-bit-propagation, propagating the information about certainly-aliveness through C++->JS->C++ boundaries.
In some cases it can take quite some time, but it reduces CC time dramatically. Per telemetry 95th percentile CYCLE_COLLECTOR_MAX_PAUSE is ~50ms, and FORGET_SKIPPABLE_MAX ~16ms, medians 6ms and 3ms)

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Olli Pettay [:smaug] from comment #3)

Yeah, it made sense for onGarbageCollection to hang off of Debugger because that's just how we do APIs exposing SpiderMonkey's internals to JS. Then I never rethought that decision with this bug.

The initial goal is to trace cycle collection in the devtools' profiler's waterfall view. I realized after hacking around a bit yesterday that instead of jumping through hoops with an onCycleCollection hook, we can do tracing the usual way here: timeline markers via AutoTimelineMarker. The onGarbageCollection hook is being used for more than just tracing GCs (it provides a unit of time for draining the allocations log), but the only thing we want from CCs is tracing.

+1 for reflection, -1 for clarity of this bug; sorry!

The only catch is that cycle collection is a global operation as opposed to being associated with a specific window/document (like the timeline profiler markers expect), so I plan to just add the CC marker to every docshell that is being traced.

So, my plan is now:

1) Maintain a `mozilla::LinkedList` of docshells actively being traced by devtools or the gecko profiler addon.

2) Add `AutoGlobalTimelineMarker` which goes through the above list and adds a timeline profiler marker to each of them.

3) Use `AutoGlobalTimelineMarker` in `nsCycleCollector::Collect` (and not differentiate between cycle collection phases, at least for now)

Comment 5

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

Note, nsCycleCollector::Collect doesn't catch forgetSkippable calls.

Comment 6

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Olli Pettay [:smaug] from comment #5)
> Note, nsCycleCollector::Collect doesn't catch forgetSkippable calls.

Would it make sense to add AutoGlobalTimelineMarker in nsCycleCollector::ForgetSkippable, too? Or is there a better place where we can catch both actions with one AutoGlobalTimelineMarker?

From some DXR digging, it appears the ForgetSkippable is called by nsJSContext::BeginCycleCollectionCallback, which is called by XPCJSRuntime::BeginCycleCollectionCallback, which is a virtual override that's called by nsCycleCollector::BeginCycleCollection, which is called by nsCycleCollector::Collect. So at least this path for ForgetSkippable *would* fall under Collect. There are a couple others, but I haven't dug into them all yet.

Comment 7

[Andrew McCreight [:mccr8]]

ForgetSkippable is mostly called on its own timer, separate from the CC. It is like a pre-CC clean up phase. The one in BeginCCCallback does the clean up in case we did not run that before.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Maintain a list of docshells whose timeline markers are being observed

Comment 9

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add mozilla::AutoGlobalTimelineMarker

Comment 10

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Trace cycle collection with AutoGlobalTimelineMarker

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Expose cycle collection markers in the devtools frontend

Comment 12

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 5: Add a test for cycle collection markers

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This patch series exposes ForgetSkippable as "Minor Cycle Collection" for webdevs. :mccr8 says that isn't quite correct and that it is more like "Cycle Collection Helper", but I just don't think that rolls off the tongue very well :P

Any alternative ideas?

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=366fc40d364b

Comment 15

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Expose cycle collection markers in the devtools frontend

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8613823 [details] [diff] [review]
Part 4: Expose cycle collection markers in the devtools frontend

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

Few things:

* I don't think we should bother labeling these events differently as minor vs normal. We can add a "Type: Collect" and "Type: ForgetSkippable" fields if inspecting the marker, but wonder how useful it is, from a bird's eye view in the waterfall, to have two distinctly different CC markers.
* This should be labeled as "CC Event", to be consistent with the "GC Event". If we want to point out one is more important (like the GC Event non-incremental), then let's do that consistently with the GC events.

For the label, can define a formatter:

function (marker) {
  let generic = L10N.getStr("timeline.label.cyclecollection");
  if (marker.name === "nsCycleCollector::Collect") {
    return `${generic} (major)`;
  }
  return generic;
}

But would rather the metadata contain extra information rather than more text on the waterfall, using a function in the fields prop as:

function (marker) {
  let type = marker.name === "nsCycleCollect::Collect" ? "Collection" : "Helper";
  return { Type: type }; // Name the property "Type" whatever should be displayed.
}

These markers are way too platform-specific to localize, if anyone did localize in the first place.

Let me know if you have any questions -- pretty much want to make it as clean as possible, considering most people don't even know what CC is. We can hide more data behind gecko platform pref if we want, too. Would be nice for a follow up to have maybe a (?) in the marker details on some of these markers explaining what CC is maybe.

Comment 17

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8613821 [details] [diff] [review]
Part 5: Add a test for cycle collection markers

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

The GC test runs GC in the chrome thread on e10s, which might not be the best way to do it -- the result is sometimes it takes a bit to trigger on e10s (so disabled on e10s for now) -- does this suffer the same issue on e10s? If it takes a few seconds longer than you'd think on e10s, put it under a conditional to not run there. Otherwise, good to go

Comment 18

[Jordan Santell [:jsantell] [@jsantell]]

Can handle any frontend stuff in bug 1128083 to not hold up this landing too, btw

Comment 19

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #17)
> Comment on attachment 8613821 [details] [diff] [review]
> Part 5: Add a test for cycle collection markers
> 
> Review of attachment 8613821 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The GC test runs GC in the chrome thread on e10s, which might not be the
> best way to do it -- the result is sometimes it takes a bit to trigger on
> e10s (so disabled on e10s for now) -- does this suffer the same issue on
> e10s? If it takes a few seconds longer than you'd think on e10s, put it
> under a conditional to not run there. Otherwise, good to go

This test doesn't have that problem: note how it is the content tab that calls SpecialPowers.Cu.forceCC() not the (chrome) test itself. Filed bug 1170609 to make the GC markers test do something similar.

Comment 20

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

Comment on attachment 8613817 [details] [diff] [review]
Part 1: Maintain a list of docshells whose timeline markers are being observed

>+#include "mozilla/Move.h"
drop this



> nsDocShell::~nsDocShell()
> {
>-  MOZ_ASSERT(!mProfileTimelineRecording);
>+  MOZ_ASSERT(!IsObserved());
Hmm, IsObserved() is quite generic name, but I don't have now better suggestions.


> NS_IMETHODIMP
> nsDocShell::SetRecordProfileTimelineMarkers(bool aValue)
> {
>   bool currentValue = nsIDocShell::GetRecordProfileTimelineMarkers();
>   if (currentValue != aValue) {
>     if (aValue) {
>       ++gProfileTimelineRecordingsCount;
>       UseEntryScriptProfiling();
>-      mProfileTimelineRecording = true;
>+
>+      MOZ_ASSERT(!mObserved);
>+      mObserved = new ObservedDocShell(
>+        mozilla::Move(nsRefPtr<nsDocShell>(this)));
>+      GetObservedDocShellsMutable().insertFront(mObserved);
>     } else {
>       --gProfileTimelineRecordingsCount;
>       UnuseEntryScriptProfiling();
>-      mProfileTimelineRecording = false;
>+
>+      delete mObserved;
>+      mObserved = nullptr;
manual deletion looks bad. Use nsAutoPtr or UniquePtr?


>@@ -261,16 +262,52 @@ public:
>   // See nsIDocShell::recordProfileTimelineMarkers
>   void AddProfileTimelineMarker(const char* aName, TracingMetadata aMetaData);
>   void AddProfileTimelineMarker(mozilla::UniquePtr<TimelineMarker>&& aMarker);
> 
>   // Global counter for how many docShells are currently recording profile
>   // timeline markers
>   static unsigned long gProfileTimelineRecordingsCount;
> 
>+  class ObservedDocShell : public mozilla::LinkedListElement<ObservedDocShell>
>+  {
>+    nsRefPtr<nsDocShell> mDocShell;
>+
>+  public:
>+    explicit ObservedDocShell(nsRefPtr<nsDocShell>&& aDocShell)
>+      : mDocShell(Move(aDocShell))
>+    { }
I'd prefer member variables at the end of class declaration.
And why all this odd Move() handling.
Just nsDocShell* aDocShell as a parameter, and then since
mDocShell is nsRefPtr it will keep the object alive.



>+  static mozilla::LinkedList<ObservedDocShell>& GetObservedDocShellsMutable()
Call this GetOrCreateObservedDocShells().
(GetOrCreate* is rather commonly used prefix in Gecko for this type of method)


I'd like to see a new patch without Move() magic which just makes code harder to read (and write).

Comment 21

[Jordan Santell [:jsantell] [@jsantell]]

Note: should add an entry to browser/devtools/performance/docs/markers.md

Comment 22

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

Comment on attachment 8613818 [details] [diff] [review]
Part 2: Add mozilla::AutoGlobalTimelineMarker

>+AutoGlobalTimelineMarker::PopulateDocShells() {
{ to its own line

>+  auto& docShells = nsDocShell::GetObservedDocShells();
auto just makes code harder to read. Please use the actual type.

>+  MOZ_ASSERT(!docShells.isEmpty());
>+
>+  for (auto* ds = docShells.getFirst(); ds; ds = ds->getNext()) {
>+    mOk = mDocShells.append(mozilla::Move(nsRefPtr<nsDocShell>(**ds)));
auto* ds makes this hard to read. I don't know what kind of thing it is.

>+  for (auto range = mDocShells.all(); !range.empty(); range.popFront()) {
>+    range.front()->AddProfileTimelineMarker(mName, TRACING_INTERVAL_START);
>+  }

sorry, hard to sell use of auto to me. It just makes code harder to read, pretty much in all the cases.
(auto is great for *writing* code, but bad for readability, IMHO)

>+AutoGlobalTimelineMarker::~AutoGlobalTimelineMarker()
>+{
>+  if (!mOk) {
>+    return;
>+  }
>+
>+  for (auto range = mDocShells.all(); !range.empty(); range.popFront()) {
and here too. Since mDocShells sounds like nsDocShell[] or such, I'd expect range to be
nsDocShell*


>+    range.front()->AddProfileTimelineMarker(mName, TRACING_INTERVAL_END);
... but apparently it is something else since nsDocShell doesn't have front()

Comment 23

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

Comment on attachment 8613819 [details] [diff] [review]
Part 3: Trace cycle collection with AutoGlobalTimelineMarker

I wonder if we should have different kinds of markers for
incremental CC slices and full CC. Though, doesn't matter usually.

Comment 24

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Maintain a list of docshells whose timeline markers are being observed

Comment 25

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add mozilla::AutoGlobalTimelineMarker

Comment 26

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Expose cycle collection markers in the devtools frontend

Comment 27

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8614302 [details] [diff] [review]
Part 4: Expose cycle collection markers in the devtools frontend

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

r+ with the below changes; your call on best name for the property to display the type of field, and whether or not we should hide the field completely when geckomode off, just strip off "nsCycleCollection", or just dump the full name. For the deep view like this, it's not going to make much sense anyway for non-gecko hackers unless we have helpers anyway

::: browser/devtools/performance/docs/markers.md
@@ +115,5 @@
>  * DOMString nonincremenetalReason - If the GC could not do an incremental
>    GC (smaller, quick GC events), and we have to walk the entire heap and
>    GC everything marked, then the reason listed here is why.
>  
> +## Cycle Collection (nsCycleCollector::Collect and nsCycleCollector::ForgetSkippable)

Would prefer these to be separate entries with their full marker names -- docs are mostly for devtools frontenders who want to know what data is on the markers themselves, before massaging, without having to dump out a bunch of markers that may have different properties (like logging a bunch of GC markers to see all the different options that are available, so one could format them)

::: browser/devtools/performance/modules/logic/marker-utils.js
@@ +419,5 @@
>      }
>    },
> +
> +  CycleCollectionLabel: function (marker) {
> +    return PREFS["show-platform-data"] ? marker.name : "Cycle Collection";

What we discussed on IRC: Let's leave the label always "Cycle Collection".

For the detailed info:

CycleCollectionFields: function (marker) {
  let Type = PREFS["show-platform-data"] ? marker.name : marker.name.replace(/nsCycleCollector/i, "");
  return { Type };
}

Or whatever field name you think is more appropriate than "Type", and can just dump the name itself here I think if geckomode on. In not geckomode, can just strip out the nsCycleCollector stuff, but still esoteric regardless, so don't feel too strongly about hiding info here (as it is details view)

::: browser/devtools/performance/modules/markers.js
@@ +122,5 @@
>    },
> +  "nsCycleCollector::Collect": {
> +    group: 1,
> +    colorName: "graphs-red",
> +    collapseFunc: collapseConsecutiveIdentical,

gonna need a rebase on these collapseFuncs

::: browser/locales/en-US/chrome/browser/devtools/timeline.properties
@@ +40,5 @@
>  timeline.label.reflow2=Layout
>  timeline.label.paint=Paint
>  timeline.label.javascript2=Function Call
> +timeline.label.cycleCollection=Cycle Collection
> +timeline.label.forgetSkippable=Minor Cycle Collection

Can remove these now

Comment 28

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

Comment on attachment 8614300 [details] [diff] [review]
Part 1: Maintain a list of docshells whose timeline markers are being observed

> #include "mozilla/EventStateManager.h"
>+#include "mozilla/Move.h"
Why you need this?

Comment 29

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

Comment on attachment 8614301 [details] [diff] [review]
Part 2: Add mozilla::AutoGlobalTimelineMarker



+  bool mOk;
Could you please add some documentation what mOk means.

>+  mozilla::Vector<nsRefPtr<nsDocShell>> mDocShells;
ugh, Vector. I guess I can live with it this time.
This is the only use of Vector in dom/ or docshell/ though.
I even thought Vector is deprecated the same way as mfbt/RefPtr

Comment 30

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Maintain a list of docshells whose timeline markers are being observed

Comment 31

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 2: Add mozilla::AutoGlobalTimelineMarker

Comment 32

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 3: Trace cycle collection with AutoGlobalTimelineMarker

Comment 33

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 4: Expose cycle collection markers in the devtools frontend

Comment 34

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 5: Add a test for cycle collection markers

Comment 35

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

This series has all review comments addressed, but after rebasing on m-c, I keep hitting crashes immediately upon starting the profiler. Will look into it tomorrow.

[NPAPI 39698] ###!!! ABORT: Aborting on channel error.: file /Users/fitzgen/src/mozilla-central/ipc/glue/MessageChannel.cpp, line 1662
#01: mozilla::ipc::MessageChannel::OnChannelErrorFromLink()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x82dea4]
#02: mozilla::ipc::ProcessLink::OnChannelError()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x82f7f1]
#03: non-virtual thunk to mozilla::ipc::ProcessLink::OnChannelError()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x82f81c]
#04: IPC::Channel::ChannelImpl::OnFileCanReadWithoutBlocking(int)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7f3fec]
#05: base::MessagePumpLibevent::OnLibeventNotification(int, short, void*)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7a648e]
#06: event_persist_closure[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7957ca]
#07: event_process_active_single_queue[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x795020]
#08: event_process_active[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7798ac]
#09: event_base_loop[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7789a7]
#10: base::MessagePumpLibevent::Run(base::MessagePump::Delegate*)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7a68cc]
#11: MessageLoop::RunInternal()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7a43b5]
#12: MessageLoop::RunHandler()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7a42c5]
#13: MessageLoop::Run()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7a426d]
#14: base::Thread::ThreadMain()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7c9686]
#15: ThreadFunc(void*)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x7ca85e]
#16: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x3268]
#17: _pthread_body[/usr/lib/system/libsystem_pthread.dylib +0x31e5]
[NPAPI 39698] ###!!! ABORT: Aborting on channel error.: file /Users/fitzgen/src/mozilla-central/ipc/glue/MessageChannel.cpp, line 1662
Hit MOZ_CRASH() at /Users/fitzgen/src/mozilla-central/memory/mozalloc/mozalloc_abort.cpp:33

Comment 36

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8614436 [details] [diff] [review]
Part 4: Expose cycle collection markers in the devtools frontend

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

With bug 1169439 landed, you might want to change the collapse function for these to be similar to the GC markers (child/parent folding, rather than consecutive-similar)

Comment 37

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Hrm, I don't get the ipc error under gdb, instead I get this crash within the profiler:

Assertion failure: strlen(mChunkList[i].get()) == mChunkLengths[i], at /Users/fitzgen/src/mozilla-central/tools/profiler/ProfileJSONWriter.cpp:44
#01: ChunkedJSONWriteFunc::CopyData() const[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4729a80]
#02: TableTicker::ToJSON(float)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x472cb9c]
#03: TableTicker::ToJSObject(JSContext*, float)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x472c9fc]
#04: mozilla_sampler_get_profile_data(JSContext*, float)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x4732978]
#05: profiler_get_profile_jsobject(JSContext*, float)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x472faaf]
#06: nsProfiler::GetProfileData(float, JSContext*, JS::MutableHandle<JS::Value>)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x472f9ff]
#07: NS_InvokeByIndex[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1c1771]
#08: CallMethodHelper::Invoke()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x10c59c8]
#09: CallMethodHelper::Call()[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x10b6241]
#10: XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x10953be]
#11: XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x1097531]
#12: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5e6b368]
#13: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5df0752]
#14: Interpret(JSContext*, js::RunState&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5e0be4b]
#15: js::RunScript(JSContext*, js::RunState&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5dfe4e2]
#16: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5df0890]
#17: js::CallOrConstructBoundFunction(JSContext*, unsigned int, JS::Value*)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x6586870]
#18: js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5e6b368]
#19: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5df0752]
#20: Interpret(JSContext*, js::RunState&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5e0be4b]
#21: js::RunScript(JSContext*, js::RunState&)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5dfe4e2]
#22: js::Invoke(JSContext*, JS::CallArgs, js::MaybeConstruct)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5df0890]
#23: js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x5dd613e]
#24: js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>)[/Users/fitzgen/src/mozilla-central/obj.noindex/dist/NightlyDebug.app/Contents/MacOS/XUL +0x616cc65]

Comment 38

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Gah, that was a stupid bug that happened while renaming methods in part 1. Here's the fix:

   static const mozilla::LinkedList<ObservedDocShell>& GetObservedDocShells()
   {
-    return GetObservedDocShells();
+    return GetOrCreateObservedDocShells();
   }

Comment 39

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 1: Maintain a list of docshells whose timeline markers are being observed

Comment 40

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Adding dep on bug 1169439 so I can rebase once that is merged to m-c.

Comment 41

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/63199013dac2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f49e64db4a66
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fff492fc48ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9becec7879dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1db2b848699a

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Backed out for browser_timelineMarkers-02.js failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=10548632&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/492669c0f158

Comment 43

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 5: Add a test for cycle collection markers

Comment 44

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

A test expected only TimeStamp markers, but we were cycle collecting in the middle of the test which screwed it up.

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7eff101c0e83

Comment 45

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Gah, none of these tests expect a cycle collection, and so they are all intermittently failing. Will need to filter the cycle collection markers out for all of them.

Comment 46

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Part 5: Add a test for cycle collection markers

Comment 47

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Alright this latest rendition filters out CC markers before passing them off to the tests that don't expect them to exist.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a3c5c53a70fa

Comment 48

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b14bf7268ea
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7ab2d47bdd9
https://hg.mozilla.org/integration/mozilla-inbound/rev/44e54f229bf8
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f7be7f07b63
https://hg.mozilla.org/integration/mozilla-inbound/rev/56ede9a5462d

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8b14bf7268ea
https://hg.mozilla.org/mozilla-central/rev/e7ab2d47bdd9
https://hg.mozilla.org/mozilla-central/rev/44e54f229bf8
https://hg.mozilla.org/mozilla-central/rev/0f7be7f07b63
https://hg.mozilla.org/mozilla-central/rev/56ede9a5462d