1249451 - nsCycleCollector::ScanRoots may cause some unexpected purple buffer usage

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

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

Attachment: patch

See https://treeherder.mozilla.org/logviewer.html#?job_id=16876870&repo=try from bug 1249226

-w patch coming too.

I think timeLog.Checkpoint("ScanRoots::listener"); doesn't provide anything useful, given that it depends on IO. And keeping to to work would have make the changes larger.

Comment 1

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

Attachment: -w

Comment 2

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

Or, hmm, do we want to be safer and run the nsConsoleService::LogStringMessage asynchronously.

Comment 3

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

Comment on attachment 8721039 [details] [diff] [review]
patch

I guess we do want the async handling.

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Olli Pettay [:smaug] (HIGH REVIEW LOAD) from comment #0)
> I think timeLog.Checkpoint("ScanRoots::listener"); doesn't provide anything
> useful, given that it depends on IO. And keeping to to work would have make
> the changes larger.

That's fine. Profiling our logging hasn't been anything we've needed to do so far.

Comment 5

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

Attachment: patch

Using nsCancelableRunnable so that the code supports workers, whether or not that is needed atm.

Comment 6

[Andrew McCreight [:mccr8]]

Comment on attachment 8721053 [details] [diff] [review]
patch

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

Thanks for fixing this.

::: xpcom/base/nsCycleCollector.cpp
@@ +1714,5 @@
> +    logFileFinalDestination->GetPath(logPath);
> +    nsAutoString msg = aCollectorKind +
> +      NS_LITERAL_STRING(" Collector log dumped to ") + logPath;
> +
> +    RefPtr<LogStringMessageAsync> log = new LogStringMessageAsync(msg);

Please add a comment about why you are doing this async.

Comment 7

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

Attachment: +comment

Comment 8

[Pulsebot]

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

Comment 9

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/78162b5666f9

Comment 10

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

and no reason why backed out :/

Comment 11

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

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

I guess the bustage was something about bc test.

Comment 12

[Andrew McCreight [:mccr8]]

Ah, right, you need an "explicit" on the ctor for LogStringMessageAsync.

Comment 13

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

Attachment: +explicit

Comment 14

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/146d77b3931d

Comment 15

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/146d77b3931d