144479 - [FIX]crash changing appearance pref when page containing position:fixed; displayed Trunk M1RC3 [@ FrameManager::UnregisterPlaceholderFrame][@ PL_DHashTableOperate]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[Christian Eyrich]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win95; de-AT; rv:1.0rc2) Gecko/20020510
BuildID:    2002051006

Changing screen resolution (e.g. from 96 dpi to 72 dpi) in preferences while
displaying above webpage.

I discovered this crash while displaying my homepage and shrinked it down and
ended up with a minimum page containing a table width align property set which
is spanned by <div style="position:fixed;"></div>.
Don't happens with position:absolute or without tables align property ...

Reproducible: Always
Steps to Reproduce:
1. Load webpage above
2. Go to Preferences and change the screen resolution
3. Hit OK

Actual Results:  crash

Expected Results:  not to crash?

Talkback crash IDs TB6272645E, TB6269864H, TB6269584W, TB6269409W, TB6269183X

Comment 1

[Frank Wein [:mcsmurf]]

wfm Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc2) Gecko/20020510

Comment 2

[guanxi]

Confirmed here: RC2 (2002051006) on Win2K
Talkback ID: TB6279006Y

Comment 3

[Claus Herwig]

Can confirm for Build 2002051408 on WinMe
Talkback ID: TB6286187X

Comment 4

[Doron Rosenberg (IBM)]

tack Signature  PL_DHashTableOperate e23d5d50
Email Address [e-mail]
Product ID MozillaTrunk
Build ID 2002051408
Trigger Time 2002-05-14 15:56:56
Platform Win32
Operating System Windows 98 4.90 build 73010104
Module XPCOM.DLL
URL visited
User Comments verifying Bugzilla Bug 144479
Trigger Reason Access violation
Source File Name pldhash.c
Trigger Line No. 481
Stack Trace
PL_DHashTableOperate [pldhash.c, line 481]
FrameManager::UnregisterPlaceholderFrame [nsFrameManager.cpp, line 810]
DoDeletingFrameSubtree [nsCSSFrameConstructor.cpp, line 9363]
DeletingFrameSubtree [nsCSSFrameConstructor.cpp, line 9426]
nsCSSFrameConstructor::RemoveFixedItems [nsCSSFrameConstructor.cpp, line 14378]
nsCSSFrameConstructor::ReconstructDocElementHierarchy
[nsCSSFrameConstructor.cpp, line 7487]
StyleSetImpl::ReconstructDocElementHierarchy [nsStyleSet.cpp, line 1507]
PresShell::ReconstructFrames [nsPresShell.cpp, line 5347]
PresShell::SetPreferenceStyleRules [nsPresShell.cpp, line 2240]
nsPresContext::PreferenceChanged [nsPresContext.cpp, line 639]
nsPresContext::PrefChangedCallback [nsPresContext.cpp, line 103]
pref_DoCallback [prefapi.cpp, line 1164]
pref_HashPref [prefapi.cpp, line 1050]
PREF_SetIntPref [prefapi.cpp, line 539]
nsPrefBranch::SetIntPref [nsPrefBranch.cpp, line 258]
nsPrefService::SetIntPref [nsPrefService.h, line 57]
nsPref::SetIntPref [nsPref.cpp, line 245]
XPTC_InvokeByIndex [xptcinvoke.cpp, line 106]
XPCWrappedNative::CallMethod [xpcwrappednative.cpp, line 1995]
XPC_WN_CallMethod [xpcwrappednativejsops.cpp, line 1267]
js_Invoke [jsinterp.c, line 790]
js_Interpret [jsinterp.c, line 2744]
js_Invoke [jsinterp.c, line 806]
js_InternalInvoke [jsinterp.c, line 881]
JS_CallFunctionValue [jsapi.c, line 3426]
nsJSContext::CallEventHandler [nsJSEnvironment.cpp, line 1045]
GlobalWindowImpl::RunTimeout [nsGlobalWindow.cpp, line 4499]
GlobalWindowImpl::TimerCallback [nsGlobalWindow.cpp, line 4846]
nsTimerImpl::Fire [nsTimerImpl.cpp, line 345]
nsTimerManager::FireNextIdleTimer [nsTimerImpl.cpp, line 591]
nsAppShell::Run [nsAppShell.cpp, line 134]
nsAppShellService::Run [nsAppShellService.cpp, line 451]
main1 [nsAppRunner.cpp, line 1472]
main [nsAppRunner.cpp, line 1808]
WinMain [nsAppRunner.cpp, line 1826]
WinMainCRTStartup()
KERNEL32.DLL + 0x1b9e4 (0xbff7b9e4)
KERNEL32.DLL + 0x1b896 (0xbff7b896)
KERNEL32.DLL + 0x1a24f (0xbff7a24f)
0x4f24242b 

Comment 5

[Matthias Versen [:Matti]]

-> Layout

Comment 6

[Christian Eyrich]

Attachment: Crash Testcase

Comment 7

[Christian Eyrich]

Forgot to test other options after fiddling out the HTML crash-code.
So here the extension:
Crash happens not only when changing screen resolution preference but when
changing any option in Appearance->Font or Appearance->Colors.

Comment 8

[Chris Petersen]

Confirming crash on OSX (2002-05-12-17 1.0.0.) using the test case and steps
provided.

Comment 9

[André Dahlqvist]

*** Bug 145648 has been marked as a duplicate of this bug. ***

Comment 10

[André Dahlqvist]

Here is another way to reproduce this:

1. Go to http://www.mozillazine.org/weblogs/asa/
2. Go to Edit->Preferences->Appearance->Fonts
3. Set Minimum Fonts Size to 12.
4. Press OK.

Stack for that crash is in the duped bug 145648.

Comment 11

[Kevin McCluskey (gone)]

Reassigning to Alex

Comment 12

[greer]

This one is making the topcrash lists for RC3 (and has shown up in all of the RC
releases). It is also happening in Trunk builds. Andre's steps in comment #10
crash for me, 100% -> topcrash+

Comment 13

[Felix Miata]

NAICT comment 7 also applies in OS/2 trunk 2002060608. The summaries and
comments some of you techies use make it tough for a mere mortal to determine
whether an observed behavior already has a bug. Maybe bug 144927 and/or bug
146891 is/are also a dupe of this?

Comment 14

[Boris Zbarsky [:bzbarsky]]

mrmazda, telling whether two crash bugs is pretty simple:

1)  Get stacks for both
2)  See if they are the same

So bug 146891 is almost definitely not a dup of this one.  The other bug you
mention has no stack, so it could be a dup of this one, sure...  if the user
edited an appearance pref (in addition to the homepage).

Comment 15

[Felix Miata]

Is "get the stacks" the same as going from the debug menu to
http://www.mozilla.org/projects/xul/tests/stacks.xul? That page is very fubar.
It extends off the bottom of the browser without a down bottom on the bottom of
the scrollbar. It doesn't look anything like the attachment to bug 144927.

Comment 16

[Felix Miata]

s/down bottom on the bottom/down button on the bottom/

Comment 17

[Boris Zbarsky [:bzbarsky]]

No, "stacks" in this context are stack traces, which can come from a debugger or
a talkback incident (if you use talkback).

Comment 18

[Felix Miata]

I guess it isn't "pretty simple" then, as I wouldn't know how to use an OS/2
debugger if I had one, and AFAIK, there is no such thing as a "talkback" build
for OS/2.

Comment 19

[Andrew Schultz]

original regression here was between 2002031208 and 2002031508

Mozilla crashes because table->ops is NULL.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Andrew, thank you!  Here is the problem:

1) nsCSSFrameConstructor::ReconstructDocElementHierarchy calls
   state.mFrameManager->ClearPlaceholderFrameMap()
2) It then calls "nsCSSFrameConstructor::RemoveFixedItems" which ends up trying
   to unregister the placeholders for those fixed-position elements.

So it seems that all we need here is a null check in
FrameManager::UnregisterPlaceholderFrame to make sure we don't try to use the
hashtable after it has been cleared by ClearPlaceholderFrameMap.  Since the
table is cleared, removing things from it is a little redundant...

Or do we want to remove the fixed items _before_ we clear the placeholder map?

Comment 21

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

A null check seems safer.

Comment 22

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

... but there should be a comment explaining why we need it, and why we
shouldn't have an assertion (if there is one now).

Although perhaps we might want to reorder the code in
ReconstructDocElementHierarchy so that we can have the assertion.  It seems safe
enough...

Comment 23

[Alexandru Savulov]

guys, feel free to take this bug from me if you're working on it :-) thx!

Comment 24

[Blake Ross]

Boris, wanna take this?

Comment 25

[Boris Zbarsky [:bzbarsky]]

Sure, but it'll likely be a week before I can get to it....

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch to fix

Fixes the testcases mentioned in this bug and looks like the right thing to do.

Comment 27

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

Comment on attachment 89515 [details] [diff] [review]
Patch to fix

r=dbaron

Comment 28

[Johnny Stenback (:jst)]

Comment on attachment 89515 [details] [diff] [review]
Patch to fix

sr=jst

Comment 29

[Boris Zbarsky [:bzbarsky]]

checked in on the trunk, mailed drivers for approval.  Marking fixed to trigger
verification....

Comment 30

[chris hofmann]

Comment on attachment 89515 [details] [diff] [review]
Patch to fix

a=chofmann for 1.01 
add the fixed1.0.1 keyword after checking in on the branch

Comment 31

[Jaime Rodriguez, Jr.]

adt1.0.1+ (on adt's behalf) approval for checkin to the 1.0 branch. pls check
this asap, then replace "mozilla1.0.1+" with "fixed1.0.1". thanks!

Comment 32

[Boris Zbarsky [:bzbarsky]]

checked in on branch.

Comment 33

[Chris Petersen]

Verified on OS X (2002-07-16-05 1.0.0) and Windows ME (2002-07-16-08 1.0.0)
branch builds.

Comment 34

[Madhur Bhatia]

v