Closed Bug 767056 Opened 12 years ago Closed 12 years ago

crash in nsRegion::Or (when I test Bug 766956, browser crashes when resize browser)

Categories

(Core :: SVG, defect)

15 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla18
Tracking Status
firefox15 + wontfix
firefox16 + verified
firefox17 + verified

People

(Reporter: alice0775, Assigned: jwatt)

References

()

Details

(Keywords: crash, regression, reproducible)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is report bp-1933edf5-00de-4de3-bfef-549ea2120621 . ============================================================= Build Identifier: http://hg.mozilla.org/mozilla-central/rev/10e019421e6b Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0a1 ID:20120621053048 http://hg.mozilla.org/releases/mozilla-aurora/rev/d557426b0c8d Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/20120621 Firefox/15.0a2 ID:20120621042006 when I test Bug 766956, browser crashes when resize browser Steps to Reproduce: 1. Open http://granite.sru.edu/~ddailey/svg/B/BBox0.svg 2. Resize browser width by dragging window side border quickly 3. Repeat step2 10-20 times Actual Results: Browser crashes Expected Results: No crash Regression window(m-c) Not crash: http://hg.mozilla.org/mozilla-central/rev/f2b2b99108a2 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517030029 Crash: http://hg.mozilla.org/mozilla-central/rev/895e12563245 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120517110214 Pusjlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f2b2b99108a2&tochange=895e12563245 Regression window(m-i) Not crash: http://hg.mozilla.org/integration/mozilla-inbound/rev/37f2536e975e Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516204627 Crash: http://hg.mozilla.org/integration/mozilla-inbound/rev/2c3647738e81 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 ID:20120516230826 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=37f2536e975e&tochange=2c3647738e81 Suspected: Bug 734082, Bug 614732
Summary: crash in nsRegion::Or → crash in nsRegion::Or (when I test Bug 766956, browser crashes when resize browser)
In local build Last Good:d3b11e443f04 First Bad;05a339620439 This crash is triggered by Bug 734082
Blocks: 734082
Assignee: nobody → jwatt
Component: SVG → Style System (CSS)
Component: Style System (CSS) → SVG
New reproducible crash in FF15, so tracking for upcoming versions. We'll likely backout bug 734082 or just wontfix if it comes down to the wire.
OS: Windows 7 → All
Hardware: x86 → All
In nsSVGOuterSVGFrame::Reflow we call nsSVGOuterSVGFrame::NotifyViewportOrTransformChanged if the size of the nsSVGOuterSVGFrame has changed. That in turn calls ChildrenOnlyTransformChanged on its children, which can then result in nsSVGUtils::ScheduleReflowSVG being called on descendants of the nsSVGOuterSVGFrame. When we've entered nsSVGOuterSVGFrame::Reflow due to a resize, we have the issue that we reflow the reflow root for the nsSVGOuterSVGFrame, but then nsSVGUtils::ScheduleReflowSVG pushes that reflow root back onto PresShell::mDirtyRoots while we're under nsSVGOuterSVGFrame::Reflow. This would lead to an infinite reflow loop, except that NotifyViewportOrTransformChanged is only called if the size of the nsSVGOuterSVGFrame changes, which prevents that from happening. What is happening when things go wrong here is that when the nsSVGOuterSVGFrame is reflowed it is first reflowed at a size assuming no scrollbars, then reflowed at a size that makes space for scrollbars. It's this oscillation that is particular to resizing that is causing us to always hit the |newViewportSize != svgElem->GetViewportSize()| block in nsSVGOuterSVGFrame::Reflow. We then end up with a stack overflow that contains the following four frames repeated: PresShell::ProcessReflowCommands(bool) PresShell::FlushPendingNotifications(mozFlushType) PresShell::HandlePostedReflowCallbacks(bool) PresShell::DidDoReflow(bool) There's no need for nsSVGUtils::ScheduleReflowSVG to call PresShell()->FrameNeedsReflow() while we're under nsSVGOuterSVGFrame::Reflow since nsSVGOuterSVGFrame::Reflow goes on to call ReflowSVG on its anonymous child before returning. The simplest fix here is to stop it from doing that.
Hmm. We're already doing that.
> Why does this list nothing? Because pushlog range listing sadly lists _pushes_ that are in the range, not changesets. And the two changesets delimiting the range are part of the same push.
(In reply to Alex Keybl [:akeybl] from comment #2) > New reproducible crash in FF15, so tracking for upcoming versions. We'll > likely backout bug 734082 or just wontfix if it comes down to the wire. At lsblakk's request, I tried backing out bug 734082 locally, but it's not a clean backout. "hg backout" required manual file-merging, so I cancelled out of that and did the following: hg up -r 05a339620439 # Go back in time to the cset we want to back out hg revert revert -r d3b11e443f04 . # Revert to its parent hg qnew backout-734082 # Save that reverting as a patch hg qpop # Pop off the patch hg up -r default # Go back to tip hg qpush # Try to reapply the backout patch The backout patch touches 33 files, and it had 15 hunks that failed to apply if I uses 8 lines of context, and 7 hunks that fail to apply if I use 3 lines of context. Fuzz fixed some, but not all, of those hunks (and with only 3 lines of context, I don't really trust patch-fuzz anyway). So if we fix this by backing out bug 734082, it'll require some manual merge conflict resolution in at least a few hunks.
(sorry -- meant to say that previous comment was entirely about the beta branch.)
Thanks dholbert. Since bug 734082 can't be cleanly backed out I'm wontfixing this for 15 and hopefully jwatt can get this cleaned up on Aurora before merge day (8/27). Thankfully this reproducible regression is a very low volume crash at the moment, but let's get this fixed for 16.
The fundamental problem here is that we have a natural infinite loop due to the way SVG is sized and the way that adding/removing scrollbars changes the dimensions of the viewport into which the SVG is fit. By way of example, lets say we have an SVG document containing the following markup: <svg width="100%" viewBox="0 0 100 100"> <!-- ... --> </svg> The width attribute being set to 100% means that we should size the SVG to give it a width 100% of the viewport. The height property defaults to 'auto' and the viewBox attribute gives the SVG an intrinsic ratio of 1:1, so the SVG/CSS rules say to set the height of the SVG to the same value as its width. Let's assume that adding a scrollbar reduces the size of the viewport by 10px. To fit this SVG into a viewport that without scrollbars is 100px wide by 95px high we'd go through the following steps: 1) viewport = 100x95: 2) svg = 100x100 3) vertical scrollbar needed 4) viewport now = 90x95 after adding vertical scrollbar 5) svg now = 90x90 6) the vertical scrollbar is not needed 7) goto step 1, and loop to death
Why didn't we have the loop before bug 734082 landed?
The code in nsHTMLScrollFrame::ReflowContents is basically responsible for carrying out the "reflow with various scrollbars/no-scrollbars permutations" steps to try and find a suitable sizing solution. It's final fallback solution (the last TryLayout() call) is to reflow with both vertical and horizontal scrollbars enabled (assuming overflow in each respective direction is not hidden). That's basically what we want here - for sizing attempts to stop with scrollbars added even though they're unnecessary (since not having them produces a size that leaves content outside the viewport without a means to scroll to it). As far as I can tell, the reason we go into a loop is because under some of the nsSVGOuterSVGFrame::Reflow() calls (triggered under nsHTMLScrollFrame::ReflowContents() as it tries reflowing at various sizes) we now hit the new nsLayoutUtils::PostRestyleEvent() call in nsSVGSVGElement::ChildrenOnlyTransformChanged(), which is passing nsChangeHint_UpdateOverflow. The stack for that: nsSVGSVGElement::ChildrenOnlyTransformChanged nsSVGOuterSVGFrame::NotifyViewportOrTransformChanged nsSVGOuterSVGFrame::Reflow nsContainerFrame::ReflowChild nsCanvasFrame::Reflow nsContainerFrame::ReflowChild nsHTMLScrollFrame::ReflowScrolledFrame nsHTMLScrollFrame::ReflowContents nsHTMLScrollFrame::Reflow nsContainerFrame::ReflowChild ViewportFrame::Reflow PresShell::DoReflow PresShell::ProcessReflowCommands When PresShell::DoReflow above returns, we then end up processing this restyle event under PresShell::DidDoReflow, and that in turn causes us to schedule a reflow under nsGfxScrollFrameInner::UpdateOverflow: PresShell::FrameNeedsReflow nsGfxScrollFrameInner::UpdateOverflow nsHTMLScrollFrame::UpdateOverflow nsCSSFrameConstructor::ProcessRestyledFrames mozilla::css::RestyleTracker::ProcessOneRestyle mozilla::css::RestyleTracker::DoProcessRestyles mozilla::css::RestyleTracker::ProcessRestyles nsCSSFrameConstructor::ProcessPendingRestyles PresShell::FlushPendingNotifications PresShell::HandlePostedReflowCallbacks PresShell::DidDoReflow PresShell::ProcessReflowCommands Unfortunately when nsCSSFrameConstructor::ProcessPendingRestyles returns, PresShell::FlushPendingNotifications then goes on to call ProcessReflowCommands(), and hence we end up back in nsHTMLScrollFrame::ReflowContents, and down into ChildrenOnlyTransformChanged with the first stack given above, but one loop deeper. And so it goes on.
Blocks: 780828
Attached patch patch (deleted) — Splinter Review
Attachment #656382 - Flags: review?(roc)
bug 778492 is likely the same issue as this one.
bug 786693 might be this too.
Flags: in-testsuite+
Target Milestone: --- → mozilla18
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
We're getting fairly frequent crash hang reports on Linux that are fixed by this.
Please nominate for aurora/beta approval once comfortable with the testing this has received on trunk.
Comment on attachment 656382 [details] [diff] [review] patch [Approval Request Comment] Bug caused by (feature/regressing bug #): 734082 User impact if declined: crashes Testing completed (on m-c, etc.): baked on m-c for several days Risk to taking this patch (and alternatives if risky): no alternatives String or UUID changes made by this patch: none
Attachment #656382 - Flags: approval-mozilla-beta?
Attachment #656382 - Flags: approval-mozilla-aurora?
Attachment #656382 - Flags: approval-mozilla-beta?
Attachment #656382 - Flags: approval-mozilla-beta+
Attachment #656382 - Flags: approval-mozilla-aurora?
Attachment #656382 - Flags: approval-mozilla-aurora+
Keywords: verifyme
Mozilla/5.0 (X11; Linux i686; rv:16.0) Gecko/20100101 Firefox/16.0 Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/20100101 Firefox/16.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:16.0) Gecko/20100101 Firefox/16.0 Verified the fix on above builds (16beta3): no crashes occurred when zooming or resizing SVG images.
Target Milestone: --- → mozilla18
Mozilla/5.0 (Windows NT 6.1; rv:17.0) Gecko/17.0 Firefox/17.0 Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17.0 Firefox/17.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:17.0) Gecko/17.0 Firefox/17.0 Fix is verified on above builds (17.0beta1), as well: no crashes occur when zooming/resizing SVGs.
QA Contact: mihaela.velimiroviciu
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: