709162 - Add crash test for cycle collector dumping

Status: RESOLVED DUPLICATE of bug 973090

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

This frequently gets broken (for the latest, see bug 709160), so we should add a very basic test that loads a simple page, does a CC dump, exits, then deletes the dump.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: call cycle collector with listener that does nothing to test CC dumping

This patch adds a test that calls the cycle collector with a listener that does nothing, to see if it crashes.

I confirmed that on trunk it crashes, and with the patch for bug 709160 it passes.

Right now this is a chrome mochitest because that's all I know how to do, but maybe it should be a crashtest?  Bobby, is there some officially sanctioned way to write a crashtest that can use chrome privileges?  I think I need those to manually invoke the cycle collector like I do.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 580782 [details] [diff] [review]
call cycle collector with listener that does nothing to test CC dumping

>+  ok(true, "Didn't run cycle collector without crashing.");

Nit: I'm not really a fan of these types of messages, since they're confusing to read when they pass (or when just reading the source). In this case it's particularly silly, because this message will never be printed in the context where it makes sense (since we'd crash first). So I'd prefer "Should be able to run CC without crashing" or somesuch. But that's not why you asked me for feedback. ;-)

> Bobby, is there some officially sanctioned way to write a crashtest that can use chrome privileges?

Judging from bug 448676, I'd say not. Realistically we need to hook SpecialPowers up to the reftest harness, but I don't know how much of a priority that is. In the mean time, I think this is fine.

Comment 3

[Andrew McCreight [:mccr8]]

Thanks for the feedback.  I'll change the comment like you suggested.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: test that we can call the CC with a listener without crashing

Another test for you to look at.

Comment 5

[Andrew McCreight [:mccr8]]

(basically identical to before, except I fixed up the message)

Comment 6

[Bobby Holley (:bholley)]

Comment on attachment 581685 [details] [diff] [review]
test that we can call the CC with a listener without crashing

>+  <a href="https://bugzilla.mozilla.org/show_bug.cgi?id="
>+     target="_blank">Mozilla Bug 709162</a>

Looks like a broken link, no?


>+  let Cu = Components.utils;
>+  let Ci = Components.interfaces;
>+
>+  var emptyListener = {
>+    QueryInterface: function QueryInterface(aIID) {
>+      if (aIID.equals(Components.interfaces.nsICycleCollectorListener) ||
>+          aIID.equals(Components.interfaces.nsISupports))

If you're going to do the Cu/Ci/Cc/Cr paradigm, you might as well be consistent and use them.

Looks fine in general. r=bholley

Comment 7

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/mozilla-central/rev/472b4a4ebea7

Comment 8

[Andrew McCreight [:mccr8]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e517d4c43143

Backed out because it caused an infinite loop with single threading of JS.  Calling JS while on the CC thread is a bad idea, apparently.  I'll rewrite the empty listener in C++ I guess.

Comment 9

[Andrew McCreight [:mccr8]]

jld is adding a test in bug 973090.

Comment 10

[Jed Davis [:jld] (away until 12-29)]

(In reply to Andrew McCreight [:mccr8] from comment #9)
> jld is adding a test in bug 973090.
> 
> *** This bug has been marked as a duplicate of bug 973090 ***

Not that it matters much, but this test which already exists might also qualify:

https://mxr.mozilla.org/mozilla-central/source/toolkit/components/aboutmemory/tests/test_aboutmemory6.xul