999446 - Hang in nsSplittableFrame::FirstContinuation nsSplittableFrame.cpp:82

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Stefan]

User Agent: 

Steps to reproduce:

1. Loaded testcase in executable compiled from mozilla-central 4811c0f6bf3f


Actual results:

1. Hang in 
nsSplittableFrame::FirstContinuation nsSplittableFrame.cpp:82


Expected results:

1. Take finite amount of time (e.g. ~3s as on ESR 17.0.11)
1. Dont block the browser (as here and on ESR 17)

Comment 1

[Stefan]

Testcase: http://www.vogtner.de/mozilla/999446/z3.html (6.0 MiB)

Comment 2

[Stefan]

(BTW: How do I get the mozilla-central revision number which matches a given Firefox release?)

Comment 3

[:Gijs (he/him)]

(In reply to Stefan from comment #2)
> (BTW: How do I get the mozilla-central revision number which matches a given
> Firefox release?)

You can open about:buildconfig in the version of Firefox you're using.

Thanks for the report. It would be useful if you could figure out a regression range for this using mozregression: https://harthur.wordpress.com/2010/09/13/mozregression-update/ (you can install using easy_install or pip, and "mozregression --help" should help you figure out the rest). I'm impressed you managed to compile it from source just because you couldn't figure out the m-c rev...!

(moving this to layout as that's where nsSplittableFrame belongs, I'm pretty sure...)

Comment 4

[Stefan]

(In reply to :Gijs Kruitbosch from comment #3)
> (In reply to Stefan from comment #2)
> > (BTW: How do I get the mozilla-central revision number which matches a given
> > Firefox release?)
> 
> You can open about:buildconfig in the version of Firefox you're using.

I have to use homebuilt versions of current releases since the recent binaries do not run here. So my problem is: How do I get the corresponding mozilla-central revision number of the Firefox-28 release binary without invoking it? AFAICS the releases are build from mozilla-releases instead of mozilla-central.

> Thanks for the report. It would be useful if you could figure out a
> regression range for this using mozregression:
> https://harthur.wordpress.com/2010/09/13/mozregression-update/ (you can
> install using easy_install or pip, and "mozregression --help" should help
> you figure out the rest). I'm impressed you managed to compile it from
> source just because you couldn't figure out the m-c rev...!

That is not the reason why I compile from source: The binaries simply do not run. That is why I cannot bisect amongst the precompiled binaries. ESR 17.0.11 and Firefox 20.0a1 (mozilla-central 868b21bed3eb) are not affected.

Comment 5

[:Gijs (he/him)]

(In reply to Stefan from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > (In reply to Stefan from comment #2)
> > > (BTW: How do I get the mozilla-central revision number which matches a given
> > > Firefox release?)
> > 
> > You can open about:buildconfig in the version of Firefox you're using.
> 
> I have to use homebuilt versions of current releases since the recent
> binaries do not run here. So my problem is: How do I get the corresponding
> mozilla-central revision number of the Firefox-28 release binary without
> invoking it? AFAICS the releases are build from mozilla-releases instead of
> mozilla-central.

I see. The releases are indeed built from mozilla-release. The tags list should help you here:

https://hg.mozilla.org/releases/mozilla-release/tags

For example, FIREFOX_28_0_RELEASE links to https://hg.mozilla.org/releases/mozilla-release/rev/5f7c149b07ba, and 28.0.1 would be FIREFOX_28_0_1_RELEASE, etc. :-)

> > Thanks for the report. It would be useful if you could figure out a
> > regression range for this using mozregression:
> > https://harthur.wordpress.com/2010/09/13/mozregression-update/ (you can
> > install using easy_install or pip, and "mozregression --help" should help
> > you figure out the rest). I'm impressed you managed to compile it from
> > source just because you couldn't figure out the m-c rev...!
> 
> That is not the reason why I compile from source: The binaries simply do not
> run. That is why I cannot bisect amongst the precompiled binaries. ESR
> 17.0.11 and Firefox 20.0a1 (mozilla-central 868b21bed3eb) are not affected.

Uhh, interesting! Do you know why the binaries do not run? Is that an issue on our end?

Comment 6

[Loic]

Regression range:
good=2014-03-12
bad=2014-03-13
Changelog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=44ae8462d6ab&tochange=46041cc216fd

Comment 7

[Stefan]

(In reply to :Gijs Kruitbosch from comment #5)
> For example, FIREFOX_28_0_RELEASE links to
> https://hg.mozilla.org/releases/mozilla-release/rev/5f7c149b07ba, and 28.0.1
> would be FIREFOX_28_0_1_RELEASE, etc. :-)

THX. I thought there was a kind of mapping from these m-r revisions to those of the "nearest" m-c revisions so that I could save cloning m-r.

> Do you know why the binaries do not run? Is that an issue on our end?

Yes and yes. :-D

Comment 8

[:Gijs (he/him)]

(In reply to Stefan from comment #7)
> (In reply to :Gijs Kruitbosch from comment #5)
> > For example, FIREFOX_28_0_RELEASE links to
> > https://hg.mozilla.org/releases/mozilla-release/rev/5f7c149b07ba, and 28.0.1
> > would be FIREFOX_28_0_1_RELEASE, etc. :-)
> 
> THX. I thought there was a kind of mapping from these m-r revisions to those
> of the "nearest" m-c revisions so that I could save cloning m-r.

This is difficult because things get uplifted, so there is no 1:1 correspondence. It's best to regression-hunt against m-c. The branch points (probably the closest to what you want) can be found in the m-c tag list as e.g. FIREFOX_AURORA_28_BASE.

> 
> > Do you know why the binaries do not run? Is that an issue on our end?
> 
> Yes and yes. :-D

Purely for my curiosity, is there a bug filed on this? :-)

---

Marking this as new and clearing the regression-window keyword, although it'd probably be helpful if we can figure out a regressing cset from the limited window (comment #6 - thanks, Loic!), which shouldn't be too hard if you have a local tree. Stefan, would you be up for doing that? :-)

Comment 9

[Alice0775 White]

I can reproduce in nWindows7 Nightly31.0a1 and Aurora30.0a2 but not in Firefox29.0b9.
Stack on hang bp-e8e883bd-8289-4858-ba8e-934332140422
              nsSplittableFrame::FirstContinuation() in Thread 0

Comment 10

[Alice0775 White]

Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f05bca38aa4d
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140311201507
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dab8e3865967
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0 ID:20140311221507
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f05bca38aa4d&tochange=dab8e3865967

Regressed by:
dab8e3865967	Simon Montagu — Use logical text layout API in nsLineLayout. Bug 789096, r=jfkthame

Comment 11

[Stefan]

(In reply to :Gijs Kruitbosch from comment #8)
> The branch points
> (probably the closest to what you want) can be found in the m-c tag list as
> e.g. FIREFOX_AURORA_28_BASE.

OK. I chose FIREFOX_AURORA_30_BASE.

> > > Do you know why the binaries do not run? Is that an issue on our end?
> > 
> > Yes and yes. :-D
> 
> Purely for my curiosity, is there a bug filed on this? :-)

Yes. Bug 723487 IIRC.

> Marking this as new and clearing the regression-window keyword, although
> it'd probably be helpful if we can figure out a regressing cset from the
> limited window (comment #6 - thanks, Loic!), which shouldn't be too hard if
> you have a local tree. Stefan, would you be up for doing that? :-)

Unfortunately not.

Comment 12

[Jonathan Kew [:jfkthame]]

:smontagu, it appears that this new code in nsLineLayout::ApplyStartMargin

  if ((pfd->mFrame->LastInFlow()->GetNextContinuation() ||
      pfd->mFrame->FrameIsNonLastInIBSplit())
    && !pfd->GetFlag(PFD_ISLETTERFRAME)) {

is problematic - in particular, FrameIsNonLastInIBSplit() calls FirstContinuation(), which iterates back through the entire list of continuations. In the testcase here, that list gets really long. :(

So I think we need to somehow avoid walking the continuation chain here - ideas?

Comment 13

[Simon Montagu :smontagu]

This is hard, and I think the original code (i.e. before the checkin dab8e3865967 from bug 789096) could calculate AvailableWidth incorrectly in some cases where the direction of the line and the span were different. There doesn't seem to be anything that tests that.

I'm also not clear why the code quoted in comment 12 causes a big performance hit in ApplyStartMargin, but doesn't do so in its original location in CanPlaceFrame.

Comment 14

[Simon Montagu :smontagu]

Attachment: tentative patch

This patch isn't perfect, but at least it stops the hang so we can move forward. There is definitely a bug in the existing code, possibly causing the overflow failure in the first line of http://test.csswg.org/suites/css2.1/20110323/html4/bidi-004.htm

Comment 15

[Jonathan Kew [:jfkthame]]

Comment on attachment 8423225 [details] [diff] [review]
tentative patch

Review of attachment 8423225 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me - resolves the hang, and even simplifies some messy bidi code a bit. Provided it passes tests, I'd say go for it.

::: layout/generic/nsLineLayout.cpp
@@ +799,5 @@
>      }
>  
> +    // Calculate whether the the frame should have a start margin and
> +    // subtract the margin from the available width if necessary.
> +    // The margin wil be applied to the starting inline coordinates of

s/wil/will/

Comment 16

[Simon Montagu :smontagu]

https://hg.mozilla.org/integration/mozilla-inbound/rev/077e1eca9084

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/077e1eca9084

Comment 18

[Lukas Blakk [:lsblakk] use ?needinfo]

Is this ready for uplift?  Please nominate with risk evaluation so we can assess whether it's worth taking this late in the FF30 Beta cycle.

Comment 19

[Simon Montagu :smontagu]

I don't think this is low-risk enough for beta

Comment 20

[Paul Silaghi, QA [:pauly]]

Reproduced the hang in Nightly 2014-04-21.
Verified fixed 32.0a1 (2014-05-26), Win 7 x64.

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Simon, what about 31 then ? :) Thanks

Comment 22

[Simon Montagu :smontagu]

Comment on attachment 8423225 [details] [diff] [review]
tentative patch

Thanks for the reminder, I meant to request earlier!

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 789096
User impact if declined: performance hit in long documents
Testing completed (on m-c, etc.): baked on m-c since 2014-05-20, and on aurora since the last uplift
Risk to taking this patch (and alternatives if risky): See comment 14. The bottom line is that this might well cause subtle changes in line breaking, particularly in bidirectional text, but after baking I'm a bit more confident that it doesn't cause serious bugs.
String or IDL/UUID changes made by this patch: none

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8423225 [details] [diff] [review]
tentative patch

Thanks! It should be part of 31 beta2.

Comment 24

[Simon Montagu :smontagu]

https://hg.mozilla.org/releases/mozilla-beta/rev/0bf7d860ef1f

Comment 25

[Catalin Varga [QA][:VarCat]]

Verified as fixed using the following environment:

FF 31.02
Build Id:20140616143923
OS: Windows 7 x64, Ubuntu 12.10 x32, Mac Os X 10.8.5