Closed Bug 1224230 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 --- unaffected
firefox45 + fixed
firefox-esr38 --- unaffected

People

(Reporter: cbook, Assigned: jfkthame)

References

()

Details

(Keywords: crash, regression)

Attachments

(3 files, 2 obsolete files)

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.
david is this something for you ?
Flags: needinfo?(dbaron)
Flags: needinfo?(dbaron)
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.
Blocks: 1122918
Flags: needinfo?(jfkthame)
[Tracking Requested - why for this release]: crash regression
(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
Attached patch Testcase (obsolete) — Splinter Review
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.)
Attachment #8687044 - Flags: review?(dbaron)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
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.
Flags: needinfo?(jfkthame)
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.
Attachment #8687044 - Flags: review?(dbaron)
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
Attachment #8687045 - Flags: review?(dbaron) → review+
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.
(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.
Attached patch TestcaseSplinter Review
OK, here's an improved version of the crashtest; this consistently crashes when it tries to modify the font-size.
Attachment #8687265 - Flags: review?(dbaron)
Attachment #8687044 - Attachment is obsolete: true
Comment on attachment 8687265 [details] [diff] [review]
Testcase

Thanks for improving it.
Attachment #8687265 - Flags: review?(dbaron) → review+
Updated patch to use a SetLineContainer accessor, etc.; carrying over r=dbaron from comment 9.
Attachment #8687045 - Attachment is obsolete: true
https://hg.mozilla.org/integration/mozilla-inbound/rev/d69c292db762bcff2ee05c9f57ae08e59c2ece41
Bug 1224230 - Explicitly store the lineContainer's writing mode in InlineIntrinsicISizeData. r=dbaron
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...
Flags: needinfo?(jfkthame)
Keywords: regression
Duplicate of this bug: 1224455
https://hg.mozilla.org/mozilla-central/rev/d69c292db762
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Group: core-security → core-security-release
Flags: needinfo?(jfkthame)
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.