Closed Bug 625311 Opened 9 years ago Closed 9 years ago
null pointer crash [@ ns
Thebes Device Context::Set DPI]
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
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?
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.
I did test strong pointer, and just starting and closing browser didn't leak. But perhaps I should test something else.
Strange. There should be a simple cycle nsThebesDeviceContext<->nsBaseWidget.
Hmm, neither of those objects have MOZ_COUNT_CTOR, I suspect...
NS_IMPL_ISUPPORTS3 is used and that gives addref/release log.
nsBaseWidget does explicitly NS_IF_RELEASE mContext
...though, if widget::Destroy isn't called, we probably leak.
As far as I see, widget->Destroy is called always. Either in viewmanager, or in nsTabChild, or in nsWebBrowser::InternalDestroy().
Assignee: nobody → Olli.Pettay
Attachment #504417 - Flags: review?(roc)
Attachment #504417 - Flags: review?(roc) → review+
Attachment #504417 - Flags: approval2.0? → approval2.0+
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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?
(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.
You need to log in before you can comment on or make changes to this bug.