Closed Bug 832611 Opened 12 years ago Closed 12 years ago

crash in mozilla::css::OverflowChangedTracker::Flush

Categories

(Core :: Layout, defect)

20 Branch
x86
Windows 7
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 + unaffected
firefox20 + fixed
firefox21 --- fixed

People

(Reporter: scoobidiver, Assigned: mattwoodrow)

References

()

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(2 files, 1 obsolete file)

This bug tracks crashes not fixed by bug 822906. See bug 822906 comment 3 for the regression range. Note that bug 815666 is tracked for 19.0 Beta. Signature mozilla::css::OverflowChangedTracker::Flush() More Reports Search UUID 644b2f56-374c-46e1-93f6-becda2130119 Date Processed 2013-01-19 06:06:24 Uptime 22 Last Crash 24 seconds before submission Install Age 4.1 hours since version was first installed. Install Time 2013-01-19 02:00:02 Product Firefox Version 21.0a1 Build ID 20130118030915 Release Channel nightly OS Windows NT OS Version 6.1.7601 Service Pack 1 Build Architecture x86 Build Architecture Info GenuineIntel family 6 model 58 stepping 9 Crash Reason EXCEPTION_ACCESS_VIOLATION_READ Crash Address 0xfffffffff0de801b App Notes AdapterVendorID: 0x1002, AdapterDeviceID: 0x679a, AdapterSubsysID: 3000174b, AdapterDriverVersion: 9.12.0.0 D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ Processor Notes sp-processor01.phx1.mozilla.com_15787:2008 EMCheckCompatibility True Adapter Vendor ID 0x1002 Adapter Device ID 0x679a Total Virtual Memory 4294836224 Available Virtual Memory 3728887808 System Memory Use Percentage 16 Available Page File 30868680704 Available Physical Memory 14310612992 Frame Module Signature Source 0 xul.dll mozilla::css::OverflowChangedTracker::Flush layout/base/RestyleTracker.h:92 1 xul.dll mozilla::css::RestyleTracker::DoProcessRestyles layout/base/RestyleTracker.cpp:248 2 xul.dll nsCSSFrameConstructor::ProcessPendingRestyles layout/base/nsCSSFrameConstructor.cpp:12160 3 xul.dll PresShell::FlushPendingNotifications layout/base/nsPresShell.cpp:3868 4 xul.dll PresShell::FlushPendingNotifications layout/base/nsPresShell.cpp:3757 5 xul.dll nsGfxScrollFrameInner::AsyncScrollPortEvent::Run layout/generic/nsGfxScrollFrame.cpp:2869 6 xul.dll PresShell::WillPaint layout/base/nsPresShell.cpp:7148 7 xul.dll nsViewManager::CallWillPaintOnObservers view/src/nsViewManager.cpp:1152 8 xul.dll nsRefreshDriver::Tick layout/base/nsRefreshDriver.cpp:955 9 xul.dll mozilla::RefreshDriverTimer::Tick layout/base/nsRefreshDriver.cpp:156 10 xul.dll nsTimerImpl::Fire xpcom/threads/nsTimerImpl.cpp:482 11 winmm.dll timeGetTime 12 xul.dll nsTimerEvent::Run xpcom/threads/nsTimerImpl.cpp:565 13 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:627 14 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:82 15 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:208 16 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:182 17 xul.dll nsBaseAppShell::Run widget/xpwidgets/nsBaseAppShell.cpp:163 18 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:232 19 xul.dll nsAppStartup::Run toolkit/components/startup/nsAppStartup.cpp:288 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 More reports at: https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Acss%3A%3AOverflowChangedTracker%3A%3AFlush%28%29
Assignee: nobody → matt.woodrow
This is still topcrash #15 over the last 3 days of builds on 21.0a1, so this seems significant.
This looks similar to bug 822906, my guess is that we're trying to access a deleted frame. Is there any way to get a URL for one of the crash reports? My guess is that this is reproducible, I just need a testcase. The previous fix removed any frames (and all descendants) when we got a nsChangeHint_ReconstructFrame hint. Do we also need to walk through placeholders when looking for descendants, or something along those lines? Is there anything else that happens during style changes that could trigger frame destruction?
(In reply to Matt Woodrow (:mattwoodrow) from comment #2) > The previous fix removed any frames (and all descendants) when we got a > nsChangeHint_ReconstructFrame hint. > > Do we also need to walk through placeholders when looking for descendants, > or something along those lines? Yes we do! > Is there anything else that happens during style changes that could trigger > frame destruction? Reconstruction of a frame F can cause nsCSSFrameConstructor to reconstruct F's ancestors actually. :-( Sorry, I should have thought of that. I think for sanity's sake we should hook up OverflowChangedTracker to nsFrame::Destroy (probably via nsCSSFrameConstructor::NotifyDestroyingFrame).
(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #4) > I crashed when hovering over one of the numbers on 22feet.in I crash also when hovering the first top left image but I don't know if it's the same crash signature as crashes are no longer submitted: bp-c31b5351-2a84-4a64-b07c-103112130124.
(In reply to Scoobidiver from comment #5) > (In reply to Robert Kaiser (:kairo@mozilla.com) from comment #4) > > I crashed when hovering over one of the numbers on 22feet.in > I crash also when hovering the first top left image but I don't know if it's > the same crash signature as crashes are no longer submitted: > bp-c31b5351-2a84-4a64-b07c-103112130124. This one has been submitted. There's some problem apparently in that it doesn't show up on crash-stats right now, but that belongs in a different bug.
Thanks for the URL's, I can reproduce the 22feet.in crash, and this fixes it.
Attachment #706239 - Flags: review?(roc)
Comment on attachment 706239 [details] [diff] [review] Use NotifyDestroyingFrame to remove frames from the tracker Review of attachment 706239 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsCSSFrameConstructor.h @@ +240,5 @@ > // ProcessRestyledFrames call in a view update batch and a script blocker. > // This function does not call ProcessAttachedQueue() on the binding manager. > // If the caller wants that to happen synchronously, it needs to handle that > // itself. > + nsresult ProcessRestyledFrames(nsStyleChangeList& aRestyleArray); I think you should keep passing aTracker in here instead of using Get/SetOverflowChangedTracker. You can set mOverflowChangedTracker internally, set it to null when the method returns, and assert it's null when this method is called. That seems simpler and less fragile... among other things it means we don't have to store mFrameConstructor in OverflowChangedTracker.
Crash Signature: [@ mozilla::css::OverflowChangedTracker::Flush()] → [@ mozilla::css::OverflowChangedTracker::Flush()] [@ nsIFrame::Properties()]
Keywords: reproducible
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #8) > I think you should keep passing aTracker in here instead of using > Get/SetOverflowChangedTracker. You can set mOverflowChangedTracker > internally, set it to null when the method returns, and assert it's null > when this method is called. > > That seems simpler and less fragile... among other things it means we don't > have to store mFrameConstructor in OverflowChangedTracker. That means that we need to require that no frames are deleted outside of ProcessRestyledFrames, but still within the scope of the OverflowChangedTracker. I think this is true currently, but I don't see any easy way to assert this going forward and prevent new bugs. Tying the NotifyDestroyingFrame hook to the lifetime of the OverflowChangedTracker seemed easier. I'll make another patch regardless in the interest of getting this landed asap :)
(In reply to Matt Woodrow (:mattwoodrow) from comment #9) > I think this is true currently, but I don't see any easy way to assert this > going forward and prevent new bugs. If this is a problem, another approach would be to have an OverflowChangedTracker permanently attached to the nsCSSFrameConstructor and notified of every frame destruction. Normally it would just be empty.
Attached file a fuzz testcase (deleted) —
Doesn't crash on inbound, so I guess it's fixed by the patch in this bug.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Amiad, please test with 21.0a1/20130129 or wait the fix lands in Aurora.
Attachment #706239 - Attachment is obsolete: true
Attachment #706239 - Flags: review?(roc)
Comment on attachment 706693 [details] [diff] [review] Use NotifyDestroyingFrame to remove frames from the tracker - alternate [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 815666 User impact if declined: Reproducible crashes on some sites. Testing completed (on m-c, etc.): Been on m-c for a few days, fixes known crashers. Risk to taking this patch (and alternatives if risky): Low risk. String or UUID changes made by this patch: None
Attachment #706693 - Flags: approval-mozilla-aurora?
Attachment #706693 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Firefox 20 beta 1 still crashes when loading http://22feet.in/ and hovering over one of the numbers: https://crash-stats.mozilla.com/report/index/bp-dacd0b24-65eb-4905-b32e-fa5ae2130226 Also, in Socorro there are several crash reports for Firefox 20 (including on the beta branch), 21 and 22: http://bit.ly/13f3aTY - for the signature [@ mozilla::css::OverflowChangedTracker::Flush()] http://bit.ly/YvXK14 - for the signature [@ nsIFrame::Properties()] Is this expected in any way?
QA Contact: simona.marcu
(In reply to Simona B [QA] from comment #19) > Firefox 20 beta 1 still crashes when loading http://22feet.in/ and hovering > over one of the numbers: Not for me.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: