1224230 - Crash @ xul!nsIFrame::InlinePrefISizeData::ForceBreak+2f [c:\users\mozilla\debug-builds\mozilla-central\layout\generic\nsframe.cpp @ 4118]

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Carsten Book [:Tomcat]]

Attachment: windbg information/stack

found on bughunter:

Steps to reproduce:
--> Load http://kinogo.co/5877-z-znachit-zahariya-2015.html

--> xul!nsIFrame::InlinePrefISizeData::ForceBreak+2f [c:\users\mozilla\debug-builds\mozilla-central\layout\generic\nsframe.cpp @ 4118]

marking as s-s because of stack and exploitable classification:

The data from the faulting address is later used as the target for a branch.

Comment 1

[Carsten Book [:Tomcat]]

david is this something for you ?

Comment 2

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

It would have been nice if something in the stack / bug had the revision of code (or date of nightly build) being tested.

Regression from bug 1122918 part 3; looks like a null-dereference of lineContainer, but I'm not 100% sure.

Comment 3

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

[Tracking Requested - why for this release]: crash regression

Comment 4

[Carsten Book [:Tomcat]]

(In reply to David Baron [:dbaron] ✈ from comment #2)
> It would have been nice if something in the stack / bug had the revision of
> code (or date of nightly build) being tested.
> 
> Regression from bug 1122918 part 3; looks like a null-dereference of
> lineContainer, but I'm not 100% sure.

hey David, i build my own debug builds so normally this are the builds from the same hour or some hours ago at max- but you are right will do that next time

Comment 5

[Jonathan Kew [:jfkthame]]

Attachment: Testcase

Here's a crashtest based on simplification of the site mentioned in comment 0. In my local testing, it doesn't hit the null-deref crash 100% of the time (presumably there's some dependency on the timing of reflow, or such like), but the frequency is high enough that it'd turn builds pretty reliably orange. (Not to be landed until the bug is fixed, obviously.)

Comment 6

[Jonathan Kew [:jfkthame]]

The problem here is that we're assuming we can get the writing mode from the lineContainer frame stored in the InlineIntrinsicISizeData, but during nsContainerFrame::DoInlineIntrinsicISize that field is temporarily cleared (when following the next-in-flow chain), so it'll be null when we visit the kids of the next-in-flow.

I'm assuming the writing mode from the (initial) lineContainer will in fact be the appropriate WM to resolve inline-start/end float positions throughout the container frame. If that's so, perhaps the easiest fix here is to store the WM itself in InlineIntrinsicISizeData, separate from the lineContainer pointer, so that we can retrieve it directly even if that pointer has been cleared.

Comment 7

[Jonathan Kew [:jfkthame]]

Attachment: Explicitly store the lineContainer's writing mode in InlineIntrinsicISizeData

Comment 8

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

Comment on attachment 8687044 [details] [diff] [review]
Testcase

>+// This doesn't always crash on initial load, so reload it a few times
>+// to improve the odds of hitting the bug.
>+function boom() {
>+  for (i = 0; i < 10; i++) {
>+    document.location.reload(false);
>+  }
>+}
>+</script>

I think you should ditch this.  I don't think it will be effective in the crashtest harness, whereas it might cause problems.

That said, why is this intermittent?  It seems like if you understand the problem, it ought to be straightforward to make a testcase that shows it 100% of the time.

Comment 9

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

Comment on attachment 8687045 [details] [diff] [review]
Explicitly store the lineContainer's writing mode in InlineIntrinsicISizeData




Could you add better comments to the definition of line and lineContainer explaining how they're nulled out for the cases where they might not be valid?

>diff --git a/layout/generic/nsRubyBaseContainerFrame.cpp b/layout/generic/nsRubyBaseContainerFrame.cpp
>       if (i == 0) {
>         data.lineContainer = aBaseISizeData->lineContainer;
>+        data.lineContainerWM = aBaseISizeData->lineContainerWM;
>         data.skipWhitespace = aBaseISizeData->skipWhitespace;
>         data.trailingWhitespace = aBaseISizeData->trailingWhitespace;
>       } else {
>         // The line container of ruby text frames is their parent,
>         // ruby text container frame.
>         data.lineContainer = frame->GetParent();
>+        data.lineContainerWM = data.lineContainer->GetWritingMode();
>       }

Maybe pull the second added line out of the if/else?  Or maybe even refactor things into a SetLineContainer method that sets both?

r=dbaron with that

Comment 10

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

Perhaps the test is intermittent because you need to force intrinsic width calculation to happen again *after* layout has happened and broken things into lines?  Seems like you could just force that.

Comment 11

[Jonathan Kew [:jfkthame]]

(In reply to David Baron [:dbaron] ✈ from comment #9)
> Maybe pull the second added line out of the if/else?  Or maybe even refactor
> things into a SetLineContainer method that sets both?
> 
> r=dbaron with that

Yes, I was thinking it might be better to make the lineContainer field private, and force all callers to use a SetLineContainer method instead, to ensure we never forget to set the writing mode at the same time. (And if SetLineContainer(nullptr) is called, it'd leave the writing mode untouched.) I'll update the patch that way.

Comment 12

[Jonathan Kew [:jfkthame]]

Attachment: Testcase

OK, here's an improved version of the crashtest; this consistently crashes when it tries to modify the font-size.

Comment 13

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

Comment on attachment 8687265 [details] [diff] [review]
Testcase

Thanks for improving it.

Comment 14

[Jonathan Kew [:jfkthame]]

Attachment: Explicitly store the lineContainer's writing mode in InlineIntrinsicISizeData

Updated patch to use a SetLineContainer accessor, etc.; carrying over r=dbaron from comment 9.

Comment 15

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d69c292db762bcff2ee05c9f57ae08e59c2ece41
Bug 1224230 - Explicitly store the lineContainer's writing mode in InlineIntrinsicISizeData. r=dbaron

Comment 16

[Jonathan Kew [:jfkthame]]

Pushed the patch to inbound.

As the test demonstrates a rather easy way to kill the browser, I'll wait a day or two until the fix has gone out in Nightly before actually landing the crashtest. Setting ni? to remind myself to land it...

Comment 18

[Phil Ringnalda (:philor)]

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

Comment 19

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f45e2942e1e1bed99c5332c98fcb31e3d901f044
Bug 1224230 - Testcase.

Comment 20

[Carsten Book [:Tomcat]]

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