Bugzilla

David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)

Comment 1

•

10 years ago

When I load this testcase, it triggers several assertions:

[37314] ###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()', file /Users/jkew/mozdev/mc-clean/layout/base/nsLayoutUtils.cpp, line 5304
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: Invalid offset: 'uint32_t(aOffset) <= mSkipChars->mCharCount', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxSkipChars.cpp, line 13
[37314] ###!!! ASSERTION: aPos out of range: 'aPos < GetLength()', file /Users/jkew/mozdev/mc-clean/gfx/thebes/gfxFont.h, line 2742

The ASAN error is expected if we have an incorrect (out-of-date?) gfxSkipChars that doesn't match the textrun involved, as suggested by the assertions here. I don't know whether the first assertion (frame tree not empty) is directly related as well, or if that's a separate problem.

Comment 2

•

10 years ago

The first assertion is likely to indicate frames getting lost from the frame tree or other similar bad things; I think it's worth investigating first.

Daniel Veditz [:dveditz]

Comment 3

•

10 years ago

until we know more calling this sec-critical based on the assertions, but could be lower if we're just reading "text" rather than objects.

Keywords: sec-critical

David Bolter [:davidb] (NeedInfo me for attention)

Comment 4

•

10 years ago

Jonathan, assigning to you for now because we love you.

Assignee: nobody → jfkthame

David Bolter [:davidb] (NeedInfo me for attention)

Comment 5

•

10 years ago

Is this tractionable?

Flags: needinfo?(jfkthame)

Flags: needinfo?(dbaron)

Comment 6

•

10 years ago

Attached file simplified testcase — Details

This is a slightly simplified version of the testcase that still hits the first assertion:

###!!! ASSERTION: frame tree not empty, but caller reported complete status: 'aSubtreeRoot->GetPrevInFlow()'

It also shows a visible rendering bug: the text "foo" that should precede the "Z" on the second line fails to appear at all.

Checking old versions of Firefox, this testcase used to render correctly, with all the text visible; it broke during the Firefox 12 cycle.

According to mozregression, the problem originated here:
  Last good revision: 34572943a3e4 (2012-01-17)
  First bad revision: f4049f65efc6 (2012-01-18)
  Pushlog:
  http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=34572943a3e4&tochange=f4049f65efc6

Simon, that pushlog range includes bug 698335, which sounds as though it might possibly be related; WDYT?

Flags: needinfo?(jfkthame) → needinfo?(smontagu)

Assignee

Comment 7

•

10 years ago

Yes, bug 698335 looks very likely. I'll look into this.

Flags: needinfo?(smontagu)

Assignee

Comment 8

•

10 years ago

Attached patch Testcases for checkin (obsolete) — Details — Splinter Review

Assignee: jfkthame → smontagu

Attachment #8411066 - Flags: review?(roc)

Assignee

Comment 9

•

10 years ago

Attached patch Patch — Details — Splinter Review

This is much the same as bug 818454 -- see bug 818454 comment 16. The patch applies the change from there to another code path where we make fluid continuations non-fluid.

The patch passed try on https://tbpl.mozilla.org/?tree=Try&rev=ad513342e18c, but I didn't want to publish the testcase on try and I don't currently have an ASAN build. Can I assume (based on comment 1) that if there are no assertions the ASAN errors have also been fixed?

Attachment #8411074 - Flags: review?(roc)

Flags: needinfo?(jfkthame)

Mats Palmgren (inactive)

Comment 10

•

10 years ago

The patch fixes the crash in my Linux64 ASAN build, fwiw.

Comment 11

•

10 years ago

(In reply to Simon Montagu :smontagu from comment #9)

> The patch passed try on https://tbpl.mozilla.org/?tree=Try&rev=ad513342e18c,
> but I didn't want to publish the testcase on try and I don't currently have
> an ASAN build. Can I assume (based on comment 1) that if there are no
> assertions the ASAN errors have also been fixed?

Yes; the gfxSkipChars assertions in the original testcase are directly related to the inconsistency that results in the ASAN error.

I can confirm that in my local build with this patch, neither testcase asserts any longer; and all the expected text is now rendered properly. IOW, looks good here.

Flags: needinfo?(jfkthame)

Robert O'Callahan (:roc) (email my personal email if necessary)

Assignee

Comment 12

•

10 years ago

Comment on attachment 8411066 [details] [diff] [review]
Testcases for checkin

(In reply to Jonathan Kew (:jfkthame) from comment #11)

> I can confirm that in my local build with this patch, neither testcase
> asserts any longer; and all the expected text is now rendered properly. IOW,
> looks good here.

Oops, I forgot that the second testcase also tests correct rendering -- I'll convert it to a reftest.

Attachment #8411066 - Attachment is obsolete: true

Attachment #8411066 - Flags: review?(roc)

Updated

•

10 years ago

Attachment #8411074 - Flags: review?(roc) → review+

Assignee

Comment 13

•

10 years ago

I believe this should be sec-moderate like other bugs with text run pointers (bug 818454, bug 826163, etc)

Flags: needinfo?(dveditz)

Assignee

Comment 14

•

10 years ago

Aurora and beta are affected, and the patch applies to them and fixes the bug.
esr24 is more complicated: the assertions don't occur, but the rendering glitch in attachment 8410437 [details] does. However, the patch doesn't fix it, and I don't immediately see anything checked in since esr24 which I would expect to make a difference.

status-firefox29: --- → affected

status-firefox30: --- → affected

status-firefox31: --- → affected

Comment 15

•

10 years ago

(In reply to Simon Montagu :smontagu from comment #14)
> esr24 is more complicated: the assertions don't occur, but the rendering
> glitch in attachment 8410437 [details] does. However, the patch doesn't fix
> it, and I don't immediately see anything checked in since esr24 which I
> would expect to make a difference.

The gfxSkipChars assertions will have been affected by my rewrite in bug 955957, which made gfxSkipCharsIterator more careful to check parameters, IIRC. The old code was less likely to assert if it was abused (although it may have silently done the wrong thing!). That landed for Fx29 (currently beta), so it's not part of esr24.

The initial "ASSERTION: frame tree not empty" wasn't dependent on that, however; I'm not sure what else is coming into play here. The fact that the rendering glitch happens seems to imply that the frame tree is getting messed up in a similar way, and this may well be leading to use of a mismatched textrun at some point, even if we're not actually hitting an assertion.

Daniel Veditz [:dveditz]

Comment 16

•

10 years ago

(In reply to Simon Montagu :smontagu from comment #9)
> ... and I don't currently have an ASAN build.

You can get Linux builds at https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/ (also available on other branches.

Blocks: 698335

Flags: needinfo?(dveditz)

Keywords: sec-critical → regression, sec-moderate

Summary: Heap-buffer-overflow in PropertyProvider::FindJustificationRange → out of bounds read in PropertyProvider::FindJustificationRange

Comment 17

•

10 years ago

(In reply to Daniel Veditz [:dveditz] from comment #16)

> You can get Linux builds at
> https://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-
> central-linux64-asan/ (also available on other branches.

...though not for esr24, AFAICT.

Comment 18

•

10 years ago

I tried the testcases here in a debug build of esr24 + the gfxSkipChars rewrite from bug 955957, in order to get the stricter assertions from that patch. Neither of the testcases here assert in that build, which implies that the ASAN out-of-bounds error also wouldn't occur, as it arises because of an out-of-range index to gfxSkipCharsIterator.

So my conclusion is that although there's clearly *some* kind of bug present in esr24 - shown by the incorrect rendering, with some of the text missing - it's not the *same* bug as reported here.

Assignee

Comment 19

•

10 years ago

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

Assignee

Comment 20

•

10 years ago

Attached patch Testcases for checkin after the bug is opened — Details — Splinter Review

Comment 21

•

10 years ago

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

Are B2G v1.2 (Gecko 26) and v1.3 (Gecko 28) affected?

Status: NEW → RESOLVED

Closed: 10 years ago

status-b2g-v1.2: --- → ?

status-b2g-v1.3: --- → ?

status-b2g-v1.4: --- → affected

status-b2g-v2.0: --- → fixed

status-firefox29: affected → wontfix

status-firefox31: affected → fixed

status-firefox-esr24: --- → wontfix

Flags: needinfo?(smontagu)

Resolution: --- → FIXED

Target Milestone: --- → mozilla31

Assignee

Comment 22

•

10 years ago

b2g 1.3 is affected and the patch fixes the bug (tested in a desktop build of source cloned from http://hg.mozilla.org/releases/mozilla-b2g28_v1_3)

b2g 1.2 is partially affected and not fixed, exactly the same as esr 24 (see comment 14 and comment 18)

Flags: needinfo?(smontagu)

Comment 23

•

10 years ago

Please request Aurora and b2g28 approval on the patch then :)

status-b2g-v1.2: ? → wontfix

status-b2g-v1.3: ? → affected

Assignee

Comment 24

•

10 years ago

Comment on attachment 8411074 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 698335
User impact if declined: potential crash
Testing completed (on m-c, etc.): Baked on trunk since 2014-04-25
Risk to taking this patch (and alternatives if risky): Some risk of regressions: this area of code has been vulnerable to regressions before (bug 722137, bug 826163). Fuzzing has been effective in finding regressions.
String or IDL/UUID changes made by this patch: N/A

Attachment #8411074 - Flags: approval-mozilla-b2g28?

Attachment #8411074 - Flags: approval-mozilla-aurora?

Lukas Blakk [:lsblakk] use ?needinfo

Comment 25

•

10 years ago

Comment on attachment 8411074 [details] [diff] [review]
Patch

Gecko 30 is on beta now, so moving the request over.

Attachment #8411074 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?

Comment 26

•

10 years ago

Comment on attachment 8411074 [details] [diff] [review]
Patch

It's early beta and if fuzzing has historically helped in finding regressions, we can take the slight risk here as we'll be on beta for 6 weeks to collect data on this.

Attachment #8411074 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Assignee

Comment 27

•

10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/3c6892869d25

status-firefox30: affected → fixed

David Bolter [:davidb] (NeedInfo me for attention)

Comment 28

•

10 years ago

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3c6892869d25

status-b2g-v1.4: affected → fixed

Updated

•

10 years ago

Flags: needinfo?(dbaron)

bhavana bajaj [:bajaj]

Updated

•

10 years ago

Attachment #8411074 - Flags: approval-mozilla-b2g28? → approval-mozilla-b2g28+

Comment 29

•

10 years ago

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/d45c46b1e23f

status-b2g-v1.3: affected → fixed