557557 - Taskbar tab preview crashes [@ mozilla::widget::TaskbarTabPreview::UpdateProxyWindowStyle()] in latest nightly when minimizing main window

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

[Jim Mathies [:jimm]]

0  	xul.dll  	mozilla::widget::TaskbarTabPreview::UpdateProxyWindowStyle  	 widget/src/windows/TaskbarTabPreview.cpp:298
1 	xul.dll 	mozilla::widget::TaskbarTabPreview::MainWindowHook 	widget/src/windows/TaskbarTabPreview.cpp:289
2 	xul.dll 	mozilla::widget::WindowHook::CallbackData::Invoke 	widget/src/windows/WindowHook.cpp:153
3 	xul.dll 	nsWindow::ProcessMessage 	
4 	xul.dll 	nsWindow::WindowProc 	widget/src/windows/nsWindow.cpp:3922

Comment 1

[:Felipe Gomes (needinfo for replies!)]

I got this twice yesterday too, both when minimizing the window. Seems to have appeared around Apr 1-2 builds.

http://crash-stats.mozilla.com/report/list?product=Firefox&platform=windows&query_search=signature&query_type=contains&query=mozilla%3A%3Awidget%3A%3ATaskbarTabPreview%3A%3AUpdateProxyWindowStyle&date=&range_value=2&range_unit=weeks&process_type=all&plugin_field=&plugin_query_type=&plugin_query=&do_query=1&signature=mozilla%3A%3Awidget%3A%3ATaskbarTabPreview%3A%3AUpdateProxyWindowStyle%28%29

Comment 2

[Martijn Wargers (dead)]

I can sort of reproduce this, but not really in a sane way.

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase, uses enhanced privileges

To reproduce:
- Open testcase in a tab, while you have at least 1 other tab open. You need to grant the privileges for the testcase.
- Press the ctrl-tab key, to switch tabs and keep the ctrl-tab key pressed to keep switching tabs
- With the mouse, go over the task bar to the entry with what corresponds to the Firefox entry where you're doing all of this to.

Comment 4

[Martijn Wargers (dead)]

Hmm, never mind the steps to reproduce.
It seems like the testcase is crashing by itself, if you let it run for a while in the background.

Comment 5

[bogas04]

This is what occurred to me : Bug 558560

Comment 7

[bogas04]

Phew , at least its not my addons :)
So i was right that it occurred due to some new feature , i guessed it to be Tabs on top, it might be , who knows.
Thanks Tyler for helping me out , and sorry for bothering you by giving wrong or long list

Comment 8

[bogas04]

(In reply to comment #1)
> I got this twice yesterday too, both when minimizing the window. Seems to have
> appeared around Apr 1-2 builds.

yes it did

Comment 9

[bogas04]

http://crash-stats.mozilla.com/report/index/c777e73f-a0a7-441c-846c-7bff32100410

Comment 10

[bogas04]

http://crash-stats.mozilla.com/report/pending/84a1ba3d-41d2-4f41-b56f-b14012100411

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Looks like we're getting WM_WINDOWPOSCHANGED for a preview that no longer exists.

Comment 12

[Rob Arnold [:robarnold]]

(In reply to comment #11)
> Looks like we're getting WM_WINDOWPOSCHANGED for a preview that no longer
> exists.

Yeah, I theorized about this to jimm last week but forgot to post it in the bug. The code to remove it in TaskbarTabPreview::DetachFromNSWindow is never actually called. The destruction code paths need to be refactored to fix this in a clean way.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Don't we just need to do http://hg.mozilla.org/mozilla-central/file/657bebceeb18/widget/src/windows/TaskbarPreview.cpp#l116 in TaskbarTabPreview?

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

I can't reproduce, but I think this will do the trick.  Try builds in a couple hours.

Comment 15

[Rob Arnold [:robarnold]]

(In reply to comment #14)
> Created an attachment (id=438363) [details]
> Patch
> 
> I can't reproduce, but I think this will do the trick.  Try builds in a couple
> hours.

That duplicates the condition check and doesn't fix the very similar as-yet-unexposed crash in TaskbarWindowPreview. What we really should do is split the destruction code path into two - one for if the window is alive and one if the window is dead. The dead path is rather trivial so perhaps that can just be inlined.

The *real* problem though is that we're invoking DetachFromNSWindow in ~TaskbarPreview - at this point the object is no longer a TaskbarTabPreview or TaskbarWindowPreview so we need to invoke it earlier.

Comment 16

[Martijn Wargers (dead)]

Is there a way to turn of this crash, by toggling a pref or something?

Comment 17

[Rob Arnold [:robarnold]]

(In reply to comment #16)
> Is there a way to turn of this crash, by toggling a pref or something?

Pref? No. You can comment out the calls to Win7Features in browser.js though and that will avoid this codepath. You can also make AeroPeek.onNewWindow/onCloseWindow no-ops:
http://mxr.mozilla.org/mozilla-central/source/browser/components/wintaskbar/WindowsPreviewPerTab.jsm#643

Comment 18

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

excuse me Rob, but when we get WM_DESTROY should not we remove all monitors from the hook regardless the message they are looking at? We should not be interested in any message after the window got destroyed. Looks like we only remove WM_DESTROY monitor there?

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

so, i suggest addition of RemoveAllMonitors, or just have RemoveMonitor check if it got WM_DESTROY, and calls clear() on data->monitors.

Comment 20

[Rob Arnold [:robarnold]]

(In reply to comment #19)
> so, i suggest addition of RemoveAllMonitors, or just have RemoveMonitor check
> if it got WM_DESTROY, and calls clear() on data->monitors.

The crash is occurring because the TaskbarPreview is disappearing before the window is destroyed (ex: you close a tab and GC happens) - no WM_DESTROY is involved. Hopefully I can get a fix up in the next few days - it's been a busy week.

Comment 21

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

oh, this is per single preview, i see

Comment 22

[Rob Arnold [:robarnold]]

Attachment: Simplify destruction handling

This is similar to Kyle's patch but goes further. I have added the precondition that mWnd == NULL to ~TaskbarPreview to ensure that any subclass must invoke TaskbarPreview::DetachFromNSWindow. After tracing the code paths, I found that the function was really only useful for setting mWnd to NULL if the window had died (via WM_DESTROY) so I removed the boolean parameter and inlined the PR_FALSE call site.

I also moved the SetVisible(PR_FALSE) call from ~TaskbarTabPreview into TaskbarTabPreview::DetachFromNSWindow since in the case that mWnd is null, it is a no-op (save for the unnoticed side effect of setting mVisible to PR_FALSE).

Alas, I could not reproduce the crash on my machine with or without the patch so we'll need to watch the crash stats to see if this disappears.

Comment 23

[Jim Mathies [:jimm]]

nits mostly - 

+  unsigned long l;

Lets use something like dwRes here.

+  SHDeleteKey(HKEY_CURRENT_USER, L"Software\\SelfRegCap");

Maybe "Software\\Mozilla\\SelfRegCap" so we get the blame if it's not cleaned up? 

+  RegOverridePredefKey(HKEY_CLASSES_ROOT, diversionKey);

Shouldn;t we check for failure here, otherwise we might register the dll proper farther down.

+    (*p)();

Do we care about the result?

Comment 24

[Jim Mathies [:jimm]]

(In reply to comment #23)
> nits mostly - 
> 
> +  unsigned long l;
> 
> Lets use something like dwRes here.
> 
> +  SHDeleteKey(HKEY_CURRENT_USER, L"Software\\SelfRegCap");
> 
> Maybe "Software\\Mozilla\\SelfRegCap" so we get the blame if it's not cleaned
> up? 
> 
> +  RegOverridePredefKey(HKEY_CLASSES_ROOT, diversionKey);
> 
> Shouldn;t we check for failure here, otherwise we might register the dll proper
> farther down.
> 
> +    (*p)();
> 
> Do we care about the result?

Bah!

wrong bug!

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 440112 [details] [diff] [review]
Simplify destruction handling

Playing with the original code, I found that after gc, the hooks for destroyed tabs kept getting called, which confirms the observations of rob & kyle. The list of win pos changed events that ended up getting fired grew quite large after a bit. With these changes in place the problem is addressed.

Comment 26

[Jim Mathies [:jimm]]

Do we need to nom. this for 3.6.5?

Comment 27

[Rob Arnold [:robarnold]]

(In reply to comment #26)
> Do we need to nom. this for 3.6.5?
I believe this is fallout from bug 520807 so we don't need to unless it gets approval.

Pushed to mozila-central:
http://hg.mozilla.org/mozilla-central/rev/e613f35c1509

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Looks like we do need this on 1.9.2 since bug 520807 is now there; it's showing up in nightly 3.6.7pre topcrashes:
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.6.7pre

Comment 29

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #28)
> Looks like we do need this on 1.9.2 since bug 520807 is now there; it's showing
> up in nightly 3.6.7pre topcrashes:
> http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.6.7pre

Looping in Beltzner and LegNeato.  We have to take this on branch for 3.6.7 since it happens with the feature prefed off.

Comment 30

[LegNeato]

Comment on attachment 440112 [details] [diff] [review]
Simplify destruction handling

a=LegNeato for 1.9.2.7

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cfb1e36d4923

Comment 32

[Ben Bucksch (:BenB)]

This fix was bad. See bug 1053839.

Comment 33

[Ben Bucksch (:BenB)]

See comment 15 here, too.