150769 - [FIXr]Crash after changing "minimum font size" and pressing OK [@ operator | nsQueryInterface::operator]

Status: VERIFIED FIXED

(Core :: Layout, defect, P1)

Description

[alge]

On linux/2002061014.
Not easily reproduceable (I tried for 10 minutes before giving up)

talkback: TB7211829H

Comment 1

[alge]

CCing asa for TB:  TB7211829H

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

operator []()
nsCOMPtr_base::assign_from_helper()
nsPresContext::PreferenceChanged()
nsPresContext::PrefChangedCallback()
pref_DoCallback()
pref_HashPref()
PREF_SetIntPref()
nsPrefBranch::SetIntPref()
nsPrefService::SetIntPref()
nsPref::SetIntPref()
XPTC_InvokeByIndex()
XPCWrappedNative::CallMethod()
XPC_WN_CallMethod()
js_Invoke()
js_Interpret()
js_Invoke()
js_InternalInvoke()
JS_CallFunctionValue()
nsJSContext::CallEventHandler()
GlobalWindowImpl::RunTimeout()
GlobalWindowImpl::TimerCallback()
nsTimerImpl::Fire()
handleTimerEvent()
PL_HandleEvent()
PL_ProcessPendingEvents()
nsEventQueueImpl::ProcessPendingEvents()
event_processor_callback()
our_gdk_io_invoke()
libglib-1.2.so.0 + 0xee00 (0x4037de00)
libglib-1.2.so.0 + 0x104c8 (0x4037f4c8)
libglib-1.2.so.0 + 0x10ad3 (0x4037fad3)
libglib-1.2.so.0 + 0x10c6c (0x4037fc6c)
libgtk-1.2.so.0 + 0x8d7f7 (0x402a07f7)
nsAppShell::Run()
nsAppShellService::Run()
main1()
main()
libc.so.6 + 0x1914f (0x4049914f)

Comment 3

[timeless]

layout

Comment 4

[Chris Petersen]

I can't reproduce in the OS X 2002-07-12-08 Build. Is this still occur in the
latest linux build ?

Comment 5

[alge]

I could never reproduce.. that doesn't mean there's not a bug, though (:

Comment 6

[Ashley Bischoff (blog at handcoding.com)]

Reopening. This crash just occured for me with 2002102508 on Win2k. Talkback ID
is TB13115185M.

(dupe of bug 144479?)

OS -> All

CC: Boris, as he applied the fix to (related?) bug 144479.

Comment 7

[Ashley Bischoff (blog at handcoding.com)]

Confirming.

Comment 8

[Boris Zbarsky [:bzbarsky]]

I've been seeing this on and off as well... stephend, could you get the stack,
possibly?

Comment 9

[[e-mail] (gone - use [e-mail] instead)]

0x0207b7fc
nsPresContext::PreferenceChanged
[d:/builds/seamonkey/mozilla/layout/base/src/nsPresContext.cpp, line 607]
nsPresContext::PrefChangedCallback
[d:/builds/seamonkey/mozilla/layout/base/src/nsPresContext.cpp, line 106]
pref_DoCallback [d:/builds/seamonkey/mozilla/modules/libpref/src/prefapi.cpp,
line 1188]
pref_HashPref [d:/builds/seamonkey/mozilla/modules/libpref/src/prefapi.cpp, line
1074]
PREF_SetIntPref [d:/builds/seamonkey/mozilla/modules/libpref/src/prefapi.cpp,
line 561]
nsPrefBranch::SetIntPref
[d:/builds/seamonkey/mozilla/modules/libpref/src/nsPrefBranch.cpp, line 259]
nsPrefService::SetIntPref
[d:/builds/seamonkey/mozilla/modules/libpref/src/nsPrefService.h, line 57]
nsPref::SetIntPref [d:/builds/seamonkey/mozilla/modules/libpref/src/nsPref.cpp,
line 245]
XPTC_InvokeByIndex
[d:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp,
line 106]
XPCWrappedNative::CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp, line 2014]
XPC_WN_CallMethod
[d:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp,
line 1282]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 841]
js_Interpret [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 2804]
js_Invoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 857]
js_InternalInvoke [d:/builds/seamonkey/mozilla/js/src/jsinterp.c, line 932]
JS_CallFunctionValue [d:/builds/seamonkey/mozilla/js/src/jsapi.c, line 3433]
nsJSContext::CallEventHandler
[d:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp, line 1044]
GlobalWindowImpl::RunTimeout
[d:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4634]
GlobalWindowImpl::TimerCallback
[d:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp, line 4981]
nsTimerImpl::Fire [d:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp,
line 368]
nsAppShell::Exit [d:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp,
line 288]
nsAppShellService::Run
[d:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp, line 472]
main1 [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1538]
main [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1886]
WinMain [d:/builds/seamonkey/mozilla/xpfe/bootstrap/nsAppRunner.cpp, line 1906]
WinMainCRTStartup()
KERNEL32.DLL + 0x1ca90 (0x77e9ca90) 

Comment 10

[Boris Zbarsky [:bzbarsky]]

ccing jst because I think he's best qualified to tell me whether I'm on crack...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: I _think_ this should fix it

I can't reproduce this now... fun... In any case, I'm thinking that the problem
here is that the mContainer goes away (gets destroyed); probably this is the
prefs dialog closing.  This calls Destroy() on the doc viewer, which calls
Destroy() on the presshell and then sets the mContainer on the prescontext to
null.

I suspect that this races with the prefchange notifications (not sure how;
maybe the event queue stuff in nsPresShell::Destroy()?) and we get the
prefchange notification when mContainer is still non-null but no longer points
to anything useful.  Which explains the weird stacks with crashes in
nsCOMPtr_base::assign_from_helper and such.

If that is the case, changing to nsWeakPtr should help the situation (not to
mention just being safer no matter what).  The other alternative would be to
bail out if mPresShell is null, of course.

Comment 12

[Kevin McCluskey (gone)]

-> Alex

Comment 13

[Kevin McCluskey (gone)]

nsbeta1+

Comment 14

[Alexandru Savulov]

Comment on attachment 104348 [details] [diff] [review]
I _think_ this should fix it

r= alexsavulov

Boris, this looks very reasonable to me. I wasn't able to repro the crash and
this seems to be the only acceptable thing to do since we (or at least I) don't
know what is the reason for the race situation. and as you say is more safer
anyway. i'll say let's put the fix in for this bug report close both bugs with
a note to reopen one of them if the crash is still there. 

(hmmm, this "Comment on the bug" field is still too wide! :-) )

Comment 15

[Boris Zbarsky [:bzbarsky]]

taking, since it's my patch...

The one issue that's possible here is a performance one.... if so, the only
place it would really matter is in font style resolution.  I'm pretty sure it
should not be a problem, but I've been unable to come up with a good way to
measure it.

Comment 16

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

Comment on attachment 104348 [details] [diff] [review]
I _think_ this should fix it

> NS_IMETHODIMP
> nsPresContext::GetContainer(nsISupports** aResult)
> {
>   NS_PRECONDITION(aResult, "null out param");
>+  nsCOMPtr<nsISupports> container = do_QueryReferent(mContainer);
>+  *aResult = container;
>+  NS_IF_ADDREF(*aResult);
>   return NS_OK;
> }

How about

NS_IMETHODIMP
nsPresContext::GetContainer(nsISupports** aResult)
{
  NS_PRECONDITION(aResult, "null out param");
  if (!mContainer) {
    *aResult = nsnull;
    return NS_OK;
  }
  return CallQueryReferent(mContainer, aResult);
}

Other than that, sr=dbaron.  (The current form is OK too, but I'm not a big fan
of the extra refcounting.)

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: good point. This uses CallQueryReferent

It seems to need the .get() to compile, though....

Comment 18

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104713 [details] [diff] [review]
good point.  This uses CallQueryReferent

marking the reviews

Comment 19

[Boris Zbarsky [:bzbarsky]]

checked in for 1.3a

Comment 20

[Alexandru Savulov]

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

Comment 21

[Frankie]

Glad this fix is coming soon. The crash still happens in Release 1.2.1

Comment 22

[timeless]

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

Comment 23

[Jay Patel [:jay]]

Adding topcrash keyword and nsQueryInterface::operator to summary for future
reference.  This crash *was* a topcrash on the MozillaTrunk.  Haven't seen any
crashes in a while though...so v.fixed.

Comment 24

[timeless]

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