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

RESOLVED FIXED in Firefox 16

Status

()

--
critical
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: alice0775, Assigned: jwatt)

Tracking

({crash, regression, reproducible})

15 Branch
mozilla18
crash, regression, reproducible
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox15+ wontfix, firefox16+ verified, firefox17+ verified)

Details

(crash signature, URL)

Attachments

(1 attachment)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Summary: crash in nsRegion::Or → crash in nsRegion::Or (when I test Bug 766956, browser crashes when resize browser)
(Reporter)

Comment 1

6 years ago
In local build
Last Good:d3b11e443f04
First Bad;05a339620439

This crash is triggered by Bug 734082
Blocks: 734082
(Assignee)

Updated

6 years ago
Assignee: nobody → jwatt
status-firefox15: --- → affected
status-firefox16: --- → affected
status-firefox17: --- → affected
tracking-firefox15: --- → ?
tracking-firefox16: --- → ?
tracking-firefox17: --- → ?
Component: SVG → Style System (CSS)
(Assignee)

Updated

6 years ago
Component: Style System (CSS) → SVG

Updated

6 years ago
tracking-firefox15: ? → +
tracking-firefox16: ? → +
tracking-firefox17: ? → +
Keywords: reproducible

Comment 2

6 years ago
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.
(Assignee)

Updated

6 years ago
OS: Windows 7 → All
Hardware: x86 → All
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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.
status-firefox15: affected → wontfix
(Assignee)

Comment 10

6 years ago
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?
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 780828
(Assignee)

Comment 13

6 years ago
Created attachment 656382 [details] [diff] [review]
patch
Attachment #656382 - Flags: review?(roc)
bug 778492 is likely the same issue as this one.
bug 786693 might be this too.

Updated

6 years ago
Duplicate of this bug: 786693
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5225ea88ba
Flags: in-testsuite+
Target Milestone: --- → mozilla18
https://hg.mozilla.org/mozilla-central/rev/bb5225ea88ba
Status: NEW → RESOLVED
Last Resolved: 6 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.
(Assignee)

Comment 21

6 years ago
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+
(Assignee)

Comment 22

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/383e1f3f6636
https://hg.mozilla.org/releases/mozilla-beta/rev/0469860cebfb
status-firefox16: affected → fixed
status-firefox17: affected → fixed
Target Milestone: mozilla18 → ---
Duplicate of this bug: 790185

Updated

6 years ago
Duplicate of this bug: 771382
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.
status-firefox16: fixed → verified

Updated

6 years ago
Target Milestone: --- → mozilla18
Duplicate of this bug: 794399
Duplicate of this bug: 794428

Updated

6 years ago
Duplicate of this bug: 794570
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.
status-firefox17: fixed → verified
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.