Closed Bug 1399453 Opened 7 years ago Closed 7 years ago

Annotate crash reports with compositor thread ownership in parent process

Categories

(Core :: Graphics, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox56 --- wontfix
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file, 2 obsolete files)

See bug 1389021. Annotate the crash reports with compositor thread owners to find out who is keeping the thread alive.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Comment on attachment 8907560 [details] [diff] [review]
0001-Bug-XXX-Annotate-crash-reports-with-compositor-threa.patch

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

Plan is to land it for a build or two, and then back out.
Attachment #8907560 - Flags: review?(dvander)
Comment on attachment 8907560 [details] [diff] [review]
0001-Bug-XXX-Annotate-crash-reports-with-compositor-threa.patch

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

::: gfx/layers/ipc/CompositorThread.cpp
@@ +43,5 @@
> +CompositorThreadHolderDebug::~CompositorThreadHolderDebug()
> +{
> +#ifdef MOZ_CRASHREPORTER
> +  if (XRE_IsParentProcess()) {
> +    CrashReporter::RemoveCrashReportAnnotation(mId);

You might want to have this change the annotation to "0" instead, that way we know *something* happened.
Attachment #8907560 - Flags: review?(dvander) → review+
try that works: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d2103975f9babf15929220bdfa2c0baa2c2f2f7

(In reply to David Anderson [:dvander] from comment #3)
> Comment on attachment 8907560 [details] [diff] [review]
> 0001-Bug-XXX-Annotate-crash-reports-with-compositor-threa.patch
> 
> Review of attachment 8907560 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/ipc/CompositorThread.cpp
> @@ +43,5 @@
> > +CompositorThreadHolderDebug::~CompositorThreadHolderDebug()
> > +{
> > +#ifdef MOZ_CRASHREPORTER
> > +  if (XRE_IsParentProcess()) {
> > +    CrashReporter::RemoveCrashReportAnnotation(mId);
> 
> You might want to have this change the annotation to "0" instead, that way
> we know *something* happened.

Good point. I was concerned about adding a lot of entries, but we shouldn't create too many content processes, so the number should be limited. I updated the patch to do this.
Attachment #8907560 - Attachment is obsolete: true
Attachment #8907603 - Flags: review+
Fix missing explicit keyword (dammit, this really should fail in my local build...) -- https://treeherder.mozilla.org/#/jobs?repo=try&revision=f25aef4e2ea9f3d2b7117a63fa7aa0293581c74c
Priority: -- → P1
Whiteboard: [gfx-noted]
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/727f0d09e299
Temporarily annotate crash reports with compositor thread owners. r=dvander
https://hg.mozilla.org/mozilla-central/rev/727f0d09e299
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Backout by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1796dd9bccfb
Backed out changeset 727f0d09e299 because the annotations are no longer needed.
Reopening as I want to reland this to help diagnose bug 1408532.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla57 → mozilla58
Attachment #8907603 - Attachment is obsolete: true
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc9d9c01080a
Temporarily annotate crash reports with compositor thread owners. r=dvander
See Also: → 1408532
Backed out for static bustage at layers/CompositorThread.h:68: bad implicit conversion constructor for 'CompositorThreadHolderDebug':

https://hg.mozilla.org/integration/mozilla-inbound/rev/91f976f5e4c5b59db6a8b1eb6bc39c3ec992774d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=dc9d9c01080a3a9d36af97706a0ec612663c48b0&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=138925228&repo=mozilla-inbound

[task 2017-10-23T16:02:19.453Z] 16:02:19     INFO -  In file included from /builds/worker/workspace/build/src/gfx/ipc/GPUParent.cpp:28:
[task 2017-10-23T16:02:19.453Z] 16:02:19     INFO -  /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/layers/CompositorThread.h:68:3: error: bad implicit conversion constructor for 'CompositorThreadHolderDebug'
[task 2017-10-23T16:02:19.454Z] 16:02:19     INFO -    CompositorThreadHolderDebug(const char* aName);
[task 2017-10-23T16:02:19.455Z] 16:02:19     INFO -    ^
Flags: needinfo?(aosmond)
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d84ea38d6fd0
Temporarily annotate crash reports with compositor thread owners. r=dvander
https://hg.mozilla.org/mozilla-central/rev/d84ea38d6fd0
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Flags: needinfo?(aosmond)
Backout by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a22351172c8
Backed out changeset d84ea38d6fd0 because the annotations are no longer needed.
You need to log in before you can comment on or make changes to this bug.