Closed Bug 861106 Opened 7 years ago Closed 7 years ago
crash in ns
Document Viewer::Set Min Font Size
It's #116 browser crasher in 20.0 and #179 in 21.0b2. It first showed up in 20.0a1/20121130 but is discontinuous across builds. The regression range might be: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=597915b66059&tochange=abb39d1df815 Signature nsDocumentViewer::SetMinFontSize(int) More Reports Search UUID 0c2572a9-d6b8-4487-8c06-94ae82130412 Date Processed 2013-04-12 09:11:55 Uptime 4781 Install Age 1.6 hours since version was first installed. Install Time 2013-04-12 07:33:15 Product Firefox Version 20.0.1 Build ID 20130409194949 Release Channel release OS Windows NT OS Version 6.1.7601 Service Pack 1 Build Architecture x86 Build Architecture Info GenuineIntel family 6 model 42 stepping 7 Crash Reason EXCEPTION_ACCESS_VIOLATION_READ Crash Address 0x0 App Notes AdapterVendorID: 0x8086, AdapterDeviceID: 0x0126, AdapterSubsysID: 04911028, AdapterDriverVersion: 22.214.171.1242 Has dual GPUs. GPU #2: AdapterVendorID2: 0x10de, AdapterDeviceID2: 0x0dcd, AdapterSubsysID2: 04911028, AdapterDriverVersion2: 126.96.36.19921D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ Processor Notes sp-processor01.phx1.mozilla.com_7073:2012; exploitability tool failed: 127 EMCheckCompatibility True Adapter Vendor ID 0x8086 Adapter Device ID 0x0126 Total Virtual Memory 4294836224 Available Virtual Memory 3527122944 System Memory Use Percentage 32 Available Page File 13966348288 Available Physical Memory 5720223744 Frame Module Signature Source 0 xul.dll nsDocumentViewer::SetMinFontSize layout/base/nsDocumentViewer.cpp:2869 1 xul.dll nsDocShell::SetupNewViewer docshell/base/nsDocShell.cpp:8156 2 xul.dll nsDocShell::Embed docshell/base/nsDocShell.cpp:6192 3 xul.dll nsDocShell::CreateContentViewer docshell/base/nsDocShell.cpp:7923 4 xul.dll nsDSURIContentListener::DoContent docshell/base/nsDSURIContentListener.cpp:122 5 xul.dll nsDocumentOpenInfo::TryContentListener uriloader/base/nsURILoader.cpp:659 6 xul.dll nsDocumentOpenInfo::DispatchContent uriloader/base/nsURILoader.cpp:360 7 xul.dll nsDocumentOpenInfo::OnStartRequest uriloader/base/nsURILoader.cpp:252 8 xul.dll mozilla::net::nsHttpChannel::CallOnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:960 9 xul.dll mozilla::net::nsHttpChannel::ContinueOnStartRequest2 netwerk/protocol/http/nsHttpChannel.cpp:4940 10 xul.dll mozilla::net::nsHttpChannel::OnStartRequest netwerk/protocol/http/nsHttpChannel.cpp:4913 11 xul.dll nsInputStreamPump::OnStateStart netwerk/base/src/nsInputStreamPump.cpp:417 12 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/src/nsInputStreamPump.cpp:368 13 xul.dll nsInputStreamReadyEvent::Run xpcom/io/nsStreamUtils.cpp:82 14 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:627 15 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:82 16 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:208 17 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:182 18 xul.dll nsBaseAppShell::Run widget/xpwidgets/nsBaseAppShell.cpp:163 19 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:154 20 xul.dll XREMain::XRE_mainRun toolkit/xre/nsAppRunner.cpp:3823 21 xul.dll XREMain::XRE_main toolkit/xre/nsAppRunner.cpp:3890 22 xul.dll XRE_main toolkit/xre/nsAppRunner.cpp:4093 23 firefox.exe do_main browser/app/nsBrowserApp.cpp:195 24 firefox.exe wmain toolkit/xre/nsWindowsWMain.cpp:105 25 firefox.exe __tmainCRTStartup crtexe.c:552 26 kernel32.dll BaseThreadInitThunk 27 ntdll.dll __RtlUserThreadStart 28 ntdll.dll _RtlUserThreadStart More reports at: https://crash-stats.mozilla.com/report/list?signature=nsDocumentViewer%3A%3ASetMinFontSize%28int%29
It looks like this is caused by a SetMinFontSize call while the document is still loading. Perhaps a race condition? If this is true, then I think the line that's causing this is this one: > mDocument->EnumerateExternalResources(SetExtResourceMinFontSize, > NS_INT32_TO_PTR(aMinFontSize)); So, we should probably check to make sure mDocument actually exists before doing this, as it seems like other functions specifically check this before accessing mDocument. Otherwise, we should probably wait until mDocument is initialized, then schedule SetMinFontSize() to be re-called.
It looks like pogo.com is sticking out from the crash reports as being an area where FF has troubles... Martijn, is there any way to construct a testcase for this? I can create a patch, but it would be nice to have some way of verifying that the patch fixes the crash.
Could bug 493879 be related to this?
Also, by 'hang', I meant the throbber is still going, as I mentioned, and the debug output is still happening, but the UI is still responsive. So, it's not really a 'hang' per se... It's more like a slowdown in the UI responsiveness.
Yeah, I guess that bug might be worksforme. However, there is bug 731049 with the same stacktrace, it seems. And apparently, the reporter can reproduce it. I'll try if I can get a reproducable testcase.
Depends on: 731049
blassey, I'm flagging you for review, since it seems as though you wrote this function... please let me know if there is someone better to review it. What this patch does it set a member variable for "pending" changes that require a document to proceed - SetTextZoom, SetFullZoom, and SetMinFontSize. Then, when mDocument changes (is assigned) in the document viewer, it "commits" these changes, essentially calling the set* function again. I'm working on getting a crashtest ready for this bug, as well, but I didn't want to delay the review based on that.
Assignee: nobody → sjohnson
Comment on attachment 743688 [details] [diff] [review] b861106 Just so that things are documented in the right places, I thought I would comment and state that I discussed this with Brad via email. Basically, there are two possible solutions to this problem (IMHO): A) Check for mDocument != nullptr, and return an error on failure, or B) Cache the value if mDocument is null, and when mDocument becomes non-null at some point later, retry the Set* function. Brad pointed out that I'm effectively implementing both solutions in this patch. This is true, and the reason for this is that if we cache the value, then, at a later time, commit these pending changes, there's a possibility that mDocument could have been set to nullptr. Thus, we would want to retain the cache (i.e. not reset the value of mPending* in the Commit() function). I communicate this failure via the NS_ERROR_* value. Brad also stated that he thought just returning the error was fine, and forego the caching of the values. This is a simpler solution, and I'm testing a patch that does exactly this ATM.
New version that just bails on null mDocument.
Attachment #744162 - Flags: review?(blassey.bugs) → review+
Target Milestone: --- → mozilla23
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.