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

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alice0775 White]

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

Comment 1

[Alice0775 White]

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

This crash is triggered by Bug 734082

Comment 2

[Alex Keybl [:akeybl]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

Hmm. We're already doing that.

Comment 5

[Jonathan Watt [:jwatt]]

Why does this list nothing?

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d3b11e443f04&tochange=05a339620439

Comment 6

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 7

[Daniel Holbert [:dholbert]]

(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.

Comment 8

[Daniel Holbert [:dholbert]]

(sorry -- meant to say that previous comment was entirely about the beta branch.)

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

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.

Comment 10

[Jonathan Watt [:jwatt]]

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

Comment 11

[Robert Longson [:longsonr]]

Why didn't we have the loop before bug 734082 landed?

Comment 12

[Jonathan Watt [:jwatt]]

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.

Comment 13

[Jonathan Watt [:jwatt]]

Attachment: patch

Comment 14

[Robert Longson [:longsonr]]

bug 778492 is likely the same issue as this one.

Comment 15

[Robert Longson [:longsonr]]

bug 786693 might be this too.

Comment 17

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb5225ea88ba

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bb5225ea88ba

Comment 19

[Robert Longson [:longsonr]]

We're getting fairly frequent crash hang reports on Linux that are fixed by this.

Comment 20

[Alex Keybl [:akeybl]]

Please nominate for aurora/beta approval once comfortable with the testing this has received on trunk.

Comment 21

[Jonathan Watt [:jwatt]]

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

Comment 22

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/383e1f3f6636
https://hg.mozilla.org/releases/mozilla-beta/rev/0469860cebfb

Comment 25

[Mihaela Velimiroviciu (:mihaelav)]

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.

Comment 29

[Mihaela Velimiroviciu (:mihaelav)]

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.

Comment 30

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old