Bugzilla

Assignee

Updated

•

11 years ago

OS: Linux → All

Hardware: x86_64 → All

Whiteboard: [reporter-external] → [reporter-external][asan]

Assignee

Comment 2

•

11 years ago

Attached file stack + frame dump — Details

The interesting part of the frame tree is the line I've marked up in
yellow.  In particalar the pink (0x625001389198) and cyan
(0x62500245a1e0) frames.  The PerFrameData for those two frames have
another PerFrameData in between which points to the destroyed frame
(marked red) which is causing the crash.  The PSD/PFD structures
appears to be alive, so I believe there used to be a span of three
frames here at some point during reflow, then the middle frame got
deleted for some reason and we failed to remove the PFD for it in
this PSD.  Note that the frame tree is correct and the line box
data too.

(I'm going to leave it in the debugger for an hour or so, in case
anyone have suggestions on other data that might be useful from
this debug session.)

Assignee

Comment 3

•

11 years ago

I've only succeeded in crashing it once in a Linux64 debug asan build
and it was quite hard.  So any help in making it more crashy and/or
reducing the test would be appreciated.

Assignee

Updated

•

11 years ago

Keywords: assertion, csectype-framepoisoning, sec-other

Assignee

Comment 4

•

11 years ago

Attached file stack + frame dump #2 — Details

Here's the exact moment when we delete that "middle of three" frames (red)
I was talking about in comment 2.  As I suspected it's a next-in-flow for
a frame that comes later in the line, so it's been reflowed already and
has a live PFD, reachable through the nsLineLayout object:
mRootSpan->mFirstFrame->mNext->mNext->mSpan->mFirstFrame->mNext

So this line contains a chain of static continuations, starting with
yellow, cyan ... blue, then that blue frame has a next-in-flow that
sits between yellow and cyan.  That doesn't look right to me.

I wonder how they ended up out-of-order like that - did bidi processing
go wrong?  or did we insert the next-in-flow in the wrong place?

Assignee

Comment 5

•

11 years ago

BTW, it's fairly easy to crash it in a Linux64 debug asan build if you start
with the test in many tabs, like so "firefox test.html test.html test.html ..."
and then wait while for the tabs to load and create frames, then switch between
tabs and occasionally delete a tab.

Simon Montagu :smontagu

Comment 6

•

11 years ago

(In reply to Mats Palmgren (:mats) from comment #4)
> So this line contains a chain of static continuations, starting with
> yellow, cyan ... blue, then that blue frame has a next-in-flow that
> sits between yellow and cyan.  That doesn't look right to me.
> 
> I wonder how they ended up out-of-order like that - did bidi processing
> go wrong?  or did we insert the next-in-flow in the wrong place?

Sounds a bit like bug 942690. Does this have the same regression range? Or does the patch there fix it?

Assignee

Comment 7

•

11 years ago

Attached file frame dump #3 — Details

It's ResolveBidi() that causes the problem.  This is the relevant part
of the frame tree before and after bidi resolution.  I think bidi
resolution is confused by the overflow list.  It should probably move it
forward so that ends up on the last static continuation 0x6250006733f8.
Or, probably simpler, drain overflow on inlines during bidi resolution.

Assignee

Comment 8

•

11 years ago

(In reply to Simon Montagu :smontagu from comment #6)
> Sounds a bit like bug 942690. Does this have the same regression range? Or
> does the patch there fix it?

I can reproduce the crash with that patch, so it appears unrelated.
I don't know if this bug is a regression or not.

Keywords: regressionwindow-wanted

Assignee

Comment 9

•

11 years ago

Attached patch wip1 (obsolete) — Details — Splinter Review

This seems to fix it for me...

Assignee: nobody → matspal

Assignee

Comment 10

•

11 years ago

Attached file frame dump with wip1 — Details

I think this is a frame tree that corresponds to the crashing one
(in dump #3 above) but with "wip1" applied so you can see the result.

Assignee

Comment 11

•

11 years ago

Attached patch part 1, implement DrainSelfOverflowList() for inlines — Details — Splinter Review

This patch just lifts the relevant code from nsInlineFrame::Reflow and
puts it in an internal helper method.  Then use that to implement
DrainSelfOverflowList() for nsInlineFrame, nsFirstLineFrame.

Attachment #8344330 - Attachment is obsolete: true

Assignee

Updated

•

11 years ago

Attachment #8344666 - Flags: review?(bzbarsky)

Assignee

Comment 12

•

11 years ago

Attached patch part 2, Drain the overflow list on inlines before traversing children for bidi processing. (obsolete) — Details — Splinter Review

https://tbpl.mozilla.org/?tree=Try&rev=da3d8da50c54

Attachment #8344670 - Flags: review?(smontagu)

Simon Montagu :smontagu

Comment 13

•

11 years ago

Comment on attachment 8344670 [details] [diff] [review]
part 2, Drain the overflow list on inlines before traversing children for bidi processing.

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

::: layout/base/nsBidiPresUtils.cpp
@@ +976,5 @@
>      PRUnichar ch = 0;
>      if (frame->IsFrameOfType(nsIFrame::eBidiInlineContainer)) {
> +      if (!(frame->GetStateBits() & NS_FRAME_FIRST_REFLOW)) {
> +        nsContainerFrame* c = static_cast<nsContainerFrame*>(frame);
> +        MOZ_ASSERT(c = do_QueryFrame(frame));

Add a message here. r=me with that.

Attachment #8344670 - Flags: review?(smontagu) → review+

Comment 14

•

11 years ago

Mats, were you able to figure out what made it non-deterministic, and/or how to make a reliable testcase?

Assignee

Comment 15

•

11 years ago

I think it's timing dependent, you'd have to interrupt reflow at just the right
moment to leave behind a tree with the right overflow list, and then have the
block marked for bidi resolution.  It's apparently very rare.

I don't know how to make the testcase more reliable, sorry.

Comment 16

•

11 years ago

I tried some variants on the following env var set, but it didn't help me reproduce the bug.

GECKO_REFLOW_INTERRUPT_MODE=counter
GECKO_REFLOW_INTERRUPT_FREQUENCY=10
GECKO_REFLOW_INTERRUPT_CHECKS_TO_SKIP=0
GECKO_REFLOW_MIN_NOINTERRUPT_DURATION=0

Comment 17

•

11 years ago

Comment on attachment 8344670 [details] [diff] [review]
part 2, Drain the overflow list on inlines before traversing children for bidi processing.

> +        MOZ_ASSERT(c = do_QueryFrame(frame));

I don't like MOZ_ASSERT expressions that have side effects.  What is this for?

Comment 18

•

11 years ago

I had trouble with GECKO_REFLOW_INTERRUPT_* in bug 949294 too. If someone can help me figure out how to use it properly, that would be great ;)

Comment 19

•

11 years ago

Properly in what sense?  There are several modes, with somewhat different goals...

Comment 20

•

11 years ago

"Find as many deterministically-reproducible bugs as possible"

Comment 21

•

11 years ago

Ah.  The settings in comment 16 are what you want at the moment, in that case: that will deterministically interrupt every 10th check.

The problem is that determining what the right value is to use for the "10" there can be complicated.

Comment 22

•

11 years ago

Comment on attachment 8344666 [details] [diff] [review]
part 1, implement DrainSelfOverflowList() for inlines

Sorry for the lag.

This generally looked reasonable, but shouldn't draining the overflow list of an inline that's at the end of a first-line end up reparenting too?  We handle that in the Reflow() case, but not in general...

r=me with that sorted out

Attachment #8344666 - Flags: review?(bzbarsky) → review+

Assignee

Comment 23

•

11 years ago

Attached patch part 1, implement DrainSelfOverflowList() for inlines, v2 — Details — Splinter Review

(In reply to Boris Zbarsky [:bz] from comment #22)
> This generally looked reasonable, but shouldn't draining the overflow list
> of an inline that's at the end of a first-line end up reparenting too?  We
> handle that in the Reflow() case, but not in general...

Maybe.  We don't do lazy parenting of nsFirstLineFrame children
though, so I'm not sure that's an issue.  But let's be conservative
and reparent style contexts for all inline overflow frames that
are descendants of a nsFirstLineFrame (which I think is what the
code in nsInlineFrame::Reflow amounts to).

Changes compared to last version:
1. reparent style contexts in nsInlineFrame::DrainSelfOverflowList
   if it's a ::first-line descendant.
2. s/eIsFirstLine/eInFirstLine/ for the flag to better reflect that
3. simplified nsFirstLineFrame::DrainSelfOverflowList to not do the
   frame reparenting stuff, since it doesn't do lazy parenting, 
   and made Reflow use this method too.
4. made nsFirstLineFrame MOZ_FINAL to avoid the vtbl lookup

I'm sceptical that we actually need to do the ReparentChildListStyle
for nsFirstLineFrame children on the overflow list.  It's been there
since the very first version of this class though (kipp, 1999-04-19)
so I'll leave it in for now:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/layout/generic/nsInlineFrame.cpp&rev=3.114&root=/cvsroot#1884
(without any corresponding bug, or explanation, unfortunately)

(Fwiw, I ran crashtests+reftests locally comparing the style context
before/after that call and it never changed.)

Attachment #8348218 - Flags: review?(bzbarsky)

Assignee

Comment 24

•

11 years ago

Attached patch part 2, Drain the overflow list on inlines before traversing children for bidi processing, v2 — Details — Splinter Review

Fixed Simon's nit.

https://tbpl.mozilla.org/?tree=Try&rev=097bf2d5fa6d

Attachment #8344670 - Attachment is obsolete: true

Attachment #8348219 - Flags: review+

Comment 25

•

11 years ago

Comment on attachment 8348218 [details] [diff] [review]
part 1, implement DrainSelfOverflowList() for inlines, v2

r=me

Attachment #8348218 - Flags: review?(bzbarsky) → review+

Assignee

Comment 26

•

11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3bcbf58111e
https://hg.mozilla.org/integration/mozilla-inbound/rev/792397422883

Flags: in-testsuite?

Assignee

Comment 27

•

11 years ago

I'll mark this testcase-wanted since we want something more reproducible
for a crashtest.

Keywords: testcase → testcase-wanted

Al Billings [:abillings - ex-MoCo]

Comment 28

•

11 years ago

https://hg.mozilla.org/mozilla-central/rev/f3bcbf58111e
https://hg.mozilla.org/mozilla-central/rev/792397422883

Status: NEW → RESOLVED

Closed: 11 years ago

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

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

status-firefox27: --- → ?

status-firefox28: --- → affected

status-firefox29: --- → fixed

status-firefox-esr24: --- → ?

Resolution: --- → FIXED

Target Milestone: --- → mozilla29

David Chan [:dchan]

Comment 29

•

10 years ago

The bounty committee has decided not to pay on this bug given that it is a framepoisoning bug. We will continue to track it with the sec-other keyword and revisit the bug if framepoisioning is determined to be exploitable.

Flags: sec-bounty? → sec-bounty-

Updated

•

10 years ago

status-firefox-esr24: ? → wontfix

Comment 30

•

10 years ago

Do we want this on Aurora/Beta/b2g26?

Flags: needinfo?(matspal)

Assignee

Comment 31

•

10 years ago

I don't think so.  The crash is not exploitable, and it was quite hard to reproduce
so it seems unlikely to occur in normal use.

Flags: needinfo?(matspal)

Paul Theriault [:pauljt] (no longer reading bugmail)

Comment 32

•

10 years ago

wontfix on the branches per comment 31

status-b2g-v1.2: ? → wontfix

status-b2g-v1.3: affected → wontfix

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

status-firefox27: ? → wontfix

status-firefox28: affected → wontfix

Updated

•

10 years ago

status-b2g18: --- → wontfix

status-b2g-v1.1hd: --- → wontfix

Al Billings [:abillings - ex-MoCo]

Updated

•

10 years ago

Whiteboard: [reporter-external][asan] → [reporter-external][asan][adv-main29-]

KTucker [:KTucker][Inactive 3/4/2016]

Assignee

Comment 34

•

9 years ago

Landed the test wrapped in an iframe:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a135f49fc628

Group: core-security

Flags: in-testsuite? → in-testsuite+

Phil Ringnalda (:philor)

Comment 35

•

9 years ago

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

Comment 36

•

9 years ago

Removing regressionwindow-wanted since this has been resolved.

QA Whiteboard: [QAnalyst-Triage+]

Keywords: regressionwindow-wanted