795284 - Write after free related to mozilla::TracerRunnable::~TracerRunnable()

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Julian Seward [:jseward]]

Multiple read- and write-after-free and double free errors observed at
shutdown on Fennec.

STR: build with profiler (SPS) support.  Start Fennec.  Start the
profiler using "Toggle Sampling" in the main menu.  Then use main menu
to quit.  There are multiple invalid memory accesses, ending in a
segfault (which may or may not be related).

Comment 1

[Julian Seward [:jseward]]

Attachment: Valgrind trace

Here is the V output.  Although there are a whole bunch of errors
reported, it is clear that they are all accesses to the same 24-byte
block that has already been freed.  So I think these all result from
the same underlying problem.

Comment 2

[Julian Seward [:jseward]]

Probably caused by incorrect use of nsCOMPtr, in CleanUpWidgetTracing
in widget/android/AndroidBridge.cpp (thanks Ms2get for confirmation):

    nsCOMPtr<TracerRunnable> sTracerRunnable;

    bool InitWidgetTracing() {
        if (!sTracerRunnable)
            sTracerRunnable = new TracerRunnable();
        return true;
    }

    void CleanUpWidgetTracing() {
        if (sTracerRunnable)
            delete sTracerRunnable;
        sTracerRunnable = nullptr;
    }

Comment 3

[Julian Seward [:jseward]]

Attachment: fix

Comment 4

[Benoit Girard (:BenWa)]

Any reason you haven't flagged review? This patch seems like it fixes a clear problem and it's ready to land.

Comment 5

[Benoit Girard (:BenWa)]

Ok blassey can review this, turns out I needed to find an intersection between the android widget peers and someone who can access this bug.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Pushed to Try.
https://tbpl.mozilla.org/?tree=Try&rev=70bffe879810

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

When did this regress? If it regressed sometime ago and affects the branches, we may first have to request sec-approval.

Comment 8

[Benoit Girard (:BenWa)]

The event tracer is only used by profiling and a few limited internal tools. While this code as shipped I don't think it's worth uplifting.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting sec-low based on comment 8. sec-approval does not need to be requested on the patch, so it can land after Try turns green.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/24b61485c945

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/24b61485c945

Comment 12

[Al Billings [:abillings - ex-MoCo]]

Did this affect Firefox 17 (and, therefore ESR 17)?