625311 - null pointer crash [@ nsThebesDeviceContext::SetDPI]

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

With the patch for Bug 624549, the following crash happens quite often

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1294906982.1294907354.19293.gz&fulltext=1

0  XUL!nsThebesDeviceContext::SetDPI [nsThebesDeviceContext.cpp:54184cfa6f0e : 650 + 0x3]
    rbx = 0x0000003c   r12 = 0x00001680   r13 = 0x5fbfbde0   r14 = 0x0000003c
    r15 = 0x5fbfbea0   rip = 0x00c61db1   rsp = 0x5fbfbd90   rbp = 0x358a1c30
    Found by: given as instruction pointer in context
 1  XUL!nsThebesDeviceContext::CheckDPIChange [nsThebesDeviceContext.cpp:54184cfa6f0e : 1190 + 0x4]
    rbx = 0x0000003c   r12 = 0x00001680   r13 = 0x03c5f560   r14 = 0x0000003c
    r15 = 0x5fbfbea0   rip = 0x00c61eb2   rsp = 0x5fbfbe20   rbp = 0x358a1c30
    Found by: call frame info
 2  XUL!nsPresContext::PreferenceChanged [nsPresContext.cpp:54184cfa6f0e : 777 + 0x8]
    rbx = 0x5fbfbf50   r12 = 0x5fbfc0c0   r13 = 0x03c5f560   r14 = 0x0000003c
    r15 = 0x5fbfbea0   rip = 0x002030ae   rsp = 0x5fbfbe40   rbp = 0x3b67b800
    Found by: call frame info




Or  0  0x0
    eip = 0x00000000   esp = 0xbf90911c   ebp = 0xbf909198   ebx = 0x034b9288
    esi = 0x0bb4c928   edi = 0x012d6a4e   eax = 0x0fd9ff10   ecx = 0x0000000c
    edx = 0x00000000   efl = 0x00010202
    Found by: given as instruction pointer in context
 1  libxul.so!nsThebesDeviceContext::CheckDPIChange [nsThebesDeviceContext.cpp : 1190 + 0xa]
    eip = 0x023b0ce3   esp = 0xbf9091a0   ebp = 0xbf9091b8
    Found by: previous frame's frame pointer
 2  libxul.so!nsPresContext::PreferenceChanged [nsPresContext.cpp : 777 + 0x17]
    eip = 0x012d66ec   esp = 0xbf9091c0   ebp = 0xbf909268
    Found by: call frame info
 3  libxul.so!nsPresContext::PrefChangedCallback [nsPresContext.cpp : 143 + 0x11]
    eip = 0x012d6ab6   esp = 0xbf909270   ebp = 0xbf909298   ebx = 0x034b9288
    esi = 0x0bb4c928
    Found by: call frame info

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The stack looks strange. There is a null check just before using mWidget.
mWidget handling is suspicious though. It is a raw pointer which is set
in the init but never unset, if I read the code correctly.

Marking ss for now.

Who knows about thebescontext?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

That does look suspicious.

We might be able to get rid of the mWidget field, and just pass the widget in where it's needed. nsScreen needs it, so nsScreen could hold a strong reference to the widget.

We can't just make mWidget a strong reference, since that would create a cycle (nsBaseWidget holds a reference to the device context). Maybe we could do that and break the cycle in nsIWidget::Destroy.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

I did test strong pointer, and just starting and closing browser didn't leak.
But perhaps I should test something else.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Strange. There should be a simple cycle nsThebesDeviceContext<->nsBaseWidget.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm, neither of those objects have MOZ_COUNT_CTOR, I suspect...

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

NS_IMPL_ISUPPORTS3 is used and that gives addref/release log.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

nsBaseWidget does explicitly NS_IF_RELEASE mContext

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

...though, if widget::Destroy isn't called, we probably leak.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: comptr

As far as I see, widget->Destroy is called always.
Either in viewmanager, or in nsTabChild, or in nsWebBrowser::InternalDestroy().

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/acb48b922e91

Comment 11

[Daniel Veditz [:dveditz]]

mWidget has been a pointer for a long time. Do we need this fix on the branches as well, or is this a regression from something that isn't going to happen on old versions? Is bug 624569 a cause, or is this just more likely to happen if we change when we run GC?

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #11)
> mWidget has been a pointer for a long time.
Since Bug 495920. I don't know what naNativeWidget was-

> or is this just more likely to happen if
> we change when we run GC?
As far as I know, this is why I noticed this.