1053839 - Taskbar preview is still crashing

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Ben Bucksch (:BenB)]

Crash stats: https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=3&signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook%28void*%2C+HWND__*%2C+unsigned+int%2C+unsigned+int%2C+long%2C+long*%29#tab-reports
Code crashing: http://mxr.mozilla.org/comm-central/source/mozilla/widget/windows/TaskbarPreview.cpp#395

Previous bugs about crashes in the same function:
Bug 557557
Bug 744992

The effects were very bad, it was crashing on startup, not even safemode would work, so these profiles and users were dead in the water.

The function is still crashing. Less than before, but this may just be because extensions stopped using this API (we certainly did, due to the bugs), or all users who had them are dead and now happy GChrome users.

The fix for bug 557557 was bad. It's still accessing preview in the problem situation.

Comment 1

[Ben Bucksch (:BenB)]

> Crash stats: https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=3&signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook%28void*%2C+HWND__*%2C+unsigned+int%2C+unsigned+int%2C+long%2C+long*%29#tab-reports

In the last month, there have been
TB 24.6: 83 crashes
TB 31 and newer: 40 crashes
FF 30 and newer: 4 crashes

Again, the low numbers may just be because extensions stopped using this API (we certainly did, due to the bugs), or all users who had them are dead and now happy GChrome users.

The FF crashes seem to be at window shutdown, which suggests bug 557557.
The TB might be 2 extensions trying to set the badge.

Comment 2

[Ben Bucksch (:BenB)]

> The fix for bug 557557 was bad. It's still accessing preview in the problem situation.

395 TaskbarPreview::MainWindowHook(void *aContext,
...
404   if (!aContext)
405     return false;
406   TaskbarPreview *preview = reinterpret_cast<TaskbarPreview*>(aContext);
407   if (nMsg == WM_DESTROY) {
408     // nsWindow is being destroyed
409     // We can't really do anything at this point including removing hooks
410     preview->mWnd = nullptr;

Here's my theory:
At window destroy time, the TaskbarPreview object has been destroyed. But Windows still holds a pointer to it. For some odd case/reason, it calls MainWindowHook() at this point, with the aContext parameter being that now wild pointer. The pointer won't be null, but an actually memory address, so the null check passes. But the line preview->mWnd = nullptr; then crashes, because it tries to access the object that now no longer exists.

This preview->mWnd = nullptr; line was fairly misguided, for that reason.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Don't do anything if the window is being destroyed

We shouldn't be messing with the preview at all on destroy. It might be a bad pointer.

Comment 4

[Mike Kaply [:mkaply]]

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

Comment 5

[Ryan VanderMeulen [:RyanVM]]

This did land on mozilla-central, though the bug didn't get marked. Sorry for any confusion.
https://hg.mozilla.org/mozilla-central/rev/b1d627e9bf5e

Comment 6

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Socorro [1] shows the following stats over the past 4 weeks:
- TB 31.2.0 = 70 crashes
- TB 31.3.0 = 84 crashes
- Firefox 34 & 35 = 0 crashes

[1] - https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=mozilla%3A%3Awidget%3A%3ATaskbarPreview%3A%3AMainWindowHook%28void%2A%2C+HWND__%2A%2C+unsigned+int%2C+unsigned+int%2C+long%2C+long%2A%29