Closed
Bug 557557
Opened 15 years ago
Closed 15 years ago
Taskbar tab preview crashes [@ mozilla::widget::TaskbarTabPreview::UpdateProxyWindowStyle()] in latest nightly when minimizing main window
Categories
(Core :: Widget: Win32, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: jimm, Assigned: robarnold)
References
Details
(Keywords: crash, topcrash, Whiteboard: [crashkill][#1 topcrash on mozilla-central (3.7a5pre)])
Crash Data
Attachments
(2 files, 1 obsolete file)
808 bytes,
text/html
|
Details | |
9.17 KB,
patch
|
jimm
:
review+
christian
:
approval1.9.2.7+
|
Details | Diff | Splinter Review |
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•15 years ago
|
||
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•15 years ago
|
||
I can sort of reproduce this, but not really in a sane way.
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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.
Updated•15 years ago
|
blocking2.0: --- → ?
Whiteboard: [crashkill] → [crashkill][#1 topcrash on mozilla-central (3.7a5pre)]
This is what occurred to me : Bug 558560
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
(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 10•15 years ago
|
||
Looks like we're getting WM_WINDOWPOSCHANGED for a preview that no longer exists.
Assignee | ||
Comment 12•15 years ago
|
||
(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.
Don't we just need to do http://hg.mozilla.org/mozilla-central/file/657bebceeb18/widget/src/windows/TaskbarPreview.cpp#l116 in TaskbarTabPreview?
I can't reproduce, but I think this will do the trick. Try builds in a couple hours.
Assignee | ||
Comment 15•15 years ago
|
||
(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.
Attachment #438363 -
Flags: review?(tellrob) → review-
Updated•15 years ago
|
Severity: normal → critical
Assignee: me → nobody
Updated•15 years ago
|
blocking2.0: ? → final+
Updated•15 years ago
|
Assignee: nobody → tellrob
Comment 16•15 years ago
|
||
Is there a way to turn of this crash, by toggling a pref or something?
Assignee | ||
Comment 17•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
so, i suggest addition of RemoveAllMonitors, or just have RemoveMonitor check if it got WM_DESTROY, and calls clear() on data->monitors.
Assignee | ||
Comment 20•15 years ago
|
||
(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•15 years ago
|
||
oh, this is per single preview, i see
Assignee | ||
Comment 22•15 years ago
|
||
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.
Attachment #438363 -
Attachment is obsolete: true
Attachment #440112 -
Flags: review?(jmathies)
![]() |
Reporter | |
Comment 23•15 years ago
|
||
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?
![]() |
Reporter | |
Comment 24•15 years ago
|
||
(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!
![]() |
Reporter | |
Comment 25•15 years ago
|
||
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.
Attachment #440112 -
Flags: review?(jmathies) → review+
![]() |
Reporter | |
Comment 26•15 years ago
|
||
Do we need to nom. this for 3.6.5?
Assignee | ||
Comment 27•15 years ago
|
||
(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
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
blocking1.9.2: --- → ?
(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.
Updated•15 years ago
|
status1.9.2:
--- → ?
Updated•15 years ago
|
Attachment #440112 -
Flags: approval1.9.2.7?
Comment 30•15 years ago
|
||
Comment on attachment 440112 [details] [diff] [review]
Simplify destruction handling
a=LegNeato for 1.9.2.7
Attachment #440112 -
Flags: approval1.9.2.7? → approval1.9.2.7+
blocking1.9.2: ? → .7+
Updated•14 years ago
|
Crash Signature: [@ mozilla::widget::TaskbarTabPreview::UpdateProxyWindowStyle()]
Comment 32•10 years ago
|
||
This fix was bad. See bug 1053839.
Comment 33•10 years ago
|
||
See comment 15 here, too.
You need to log in
before you can comment on or make changes to this bug.
Description
•