Print preview crash when changing layout. [@ BCMapCellInfo::SetColumn(int) ] and [@ nsTableFrame::CalcBCBorders() ]

NEW
Assigned to

Status

()

Core
Layout: Tables
--
critical
3 years ago
2 years ago

People

(Reporter: Fabien Ménager, Assigned: mats)

Tracking

({crash, regression, testcase})

24 Branch
x86_64
All
crash, regression, testcase
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tbird crash], crash signature)

Attachments

(2 attachments)

(Reporter)

Description

3 years ago
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20141126041045

Steps to reproduce:

I open a page with big tables, and open print preview. 
I switch from portrait layout to landscape, and it crashes.
I tried in safe mode, the same issue happens.


Actual results:

It crashes, with this report : 
https://crash-stats.mozilla.com/report/index/fac86b7f-e67d-4c39-9a7b-872aa2150105


Expected results:

Don't crash.
I can't join a sample as it contains sensitive data, please tell me if you need it to solve this issue, I'll try to anonymize it.
(Reporter)

Updated

3 years ago
Keywords: crash
(Reporter)

Comment 1

3 years ago
Created attachment 8544596 [details]
Simple test case (as simple as I could)

FYI, it works when I remove this line :

    src: url()

Or this one :

    table.tbl td.first_perf{background:red;}

The test case crashes when no printer was selected before, when using the print preview.

Comment 2

3 years ago
2015-01-12-03-02-01-mozilla-central-firefox-37.0a1.ru.linux-x86_64

Reproduced with the testcase: bp-7113af1a-9d37-4a4a-a90d-19c9b2150112 [@ nsTableFrame::CalcBCBorders() ]
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsTableFrame::CalcBCBorders() ] [@ BCMapCellInfo::SetColumn(int) ]
QA Whiteboard: [bugday-20140112]
Component: Untriaged → Layout: Tables
Ever confirmed: true
Keywords: crashreportid, testcase
OS: Windows 8.1 → All
Product: Firefox → Core

Comment 3

3 years ago
regressionwindow
w 2013-06-19-03-10-48-mozilla-central-firefox-24.0a1.en-US.linux-x86_64
w 2013-06-21-03-10-51-mozilla-central-firefox-24.0a1.en-US.linux-x86_64
w 2013-06-22-03-10-42-mozilla-central-firefox-24.0a1.en-US.linux-x86_64 cea75ce9a559

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=cea75ce9a559&tochange=4c4f75c20e9b

b 2013-06-23-03-10-22-mozilla-central-firefox-24.0a1.en-US.linux-x86_64 4c4f75c20e9b [@ libxul.so@0xfd5e49 | libxul.so@0xfd7c1f | libxul.so@0xfd7d0f | libxul.so@0xfd8190 | libxul.so@0xfd8289 | libxul.so@0xfd9374 | libxul.so@0xfd93d3 | libxul.so@0xe38ce9 | libxul.so@0x185bff6 | libxul.so@0x18594ea | libxul.so@0x185992f | libxul.so@0x18595a... ]
b 2013-07-19-03-02-04-mozilla-central-firefox-25.0a1.en-US.linux-x86_64
b 2013-11-05-03-02-06-mozilla-central-firefox-28.0a1.en-US.linux-x86_64
b 2015-01-12-03-02-01-mozilla-central-firefox-37.0a1.ru.linux-x86_64
Keywords: regression
Summary: Print preview crash when changing layout [@ BCMapCellInfo::SetColumn(int) ] → Print preview crash when changing layout. [@ BCMapCellInfo::SetColumn(int) ] and [@ nsTableFrame::CalcBCBorders() ]

Updated

3 years ago
Version: 34 Branch → 24 Branch

Updated

3 years ago
QA Whiteboard: [bugday-20140112] → [bugday-20150112]
Comment hidden (spam)
(Reporter)

Comment 5

3 years ago
May I ask you if there is something we can change in our HTML to workaround this problem quickly ?
(Reporter)

Comment 6

3 years ago
Any news about this ?
(Assignee)

Comment 7

3 years ago
I can reproduce this crash in Print Preview when switching from Portrait to Landscape.
There are quite a few assertions before the crash occurs (Linux64 debug build):

ASSERTION: should not be trying to restyle this frame separately: '!GetPrevContinuationWithSameStyle(mFrame)', layout/base/RestyleManager.cpp, line 2698
ASSERTION: GetColFrame called on next in flow: '!GetPrevInFlow()', layout/tables/nsTableFrame.cpp, line 431
ASSERTION: invalid col index: 'Error', layout/tables/nsTableFrame.cpp, line 437
ASSERTION: CellIterator program error: 'false', layout/tables/nsTableFrame.cpp, line 4432
ASSERTION: GetColFrame called on next in flow: '!GetPrevInFlow()', layout/tables/nsTableFrame.cpp, line 431
ASSERTION: invalid col index: 'Error', layout/tables/nsTableFrame.cpp, line 437
ASSERTION: null mCurrentColFrame: 'Error', layout/tables/nsTableFrame.cpp, line 5575

(gdb) bt
#0  nsIFrame::GetParent (this=0x0) at layout/generic/nsIFrame.h:618
#1  BCMapCellInfo::SetColumn at layout/tables/nsTableFrame.cpp:5578
#2  nsTableFrame::CalcBCBorders at layout/tables/nsTableFrame.cpp:5774
#3  nsDelayedCalcBCBorders::Run at layout/tables/nsTableFrame.cpp:4767
#4  nsThread::ProcessNextEvent at xpcom/threads/nsThread.cpp:846
...
Assignee: nobody → mats
(Assignee)

Comment 8

3 years ago
Created attachment 8613219 [details] [diff] [review]
fix

Make the GetColFrame calls on the first-in-flow table frame.
This fixes the assertions and crash, except for the first assertion
which is bug 927267.

(BCMapCellInfo is a temporary struct that is only used while calculating
the border-collapse border resolution and such, so its size isn't
important.  I introduce a 'mTableFirstInFlow' so that FirstInFlow()
is only called once (to avoid virtual calls).  We already do the same
in BCPaintBorderIterator.)

I failed to make an automated crashtest that would actually crash.
I suspect that the standard 5x3 inch reftest page size doesn't
trigger the condition needed, or something.  The test does trigger
the style system assertion though but I figured we can land that
in bug 927267.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff9faa0d2a1e
Attachment #8613219 - Flags: review?(quanxunzhen)
Comment on attachment 8613219 [details] [diff] [review]
fix

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

Overall it looks good to me, but neither you nor I am a peer of layout engine. Redirect the review request to :dbaron.

The border-collapse code was generally reviewed by :roc, but he doesn't seem to have much time online in the following week.

::: layout/tables/nsTableFrame.cpp
@@ +430,5 @@
>  {
>    NS_ASSERTION(!GetPrevInFlow(), "GetColFrame called on next in flow");
>    int32_t numCols = mColFrames.Length();
>    if ((aColIndex >= 0) && (aColIndex < numCols)) {
> +    MOZ_ASSERT(mColFrames.ElementAt(aColIndex));

It doesn't seem to me that the pointer in mColFrames is nullable. The only insertion call is immediately followed by a method call on the inserted pointer.

@@ +5572,5 @@
>  
>  void
>  BCMapCellInfo::SetColumn(int32_t aColX)
>  {
> +  mCurrentColFrame = mTableFirstInFlow->GetColFrame(aColX);

If you remove the null-check here, probably you can also remove the checks in BCMapCellInfo::SetInfo()?
Attachment #8613219 - Flags: review?(quanxunzhen)
Attachment #8613219 - Flags: review?(dbaron)
Attachment #8613219 - Flags: feedback+
Attachment #8613219 - Flags: review?(dbaron) → review+

Updated

2 years ago
Crash Signature: [@ nsTableFrame::CalcBCBorders() ] [@ BCMapCellInfo::SetColumn(int) ] → [@ nsTableFrame::CalcBCBorders() ] [@ BCMapCellInfo::SetColumn(int) ] [@ nsTableFrame::CalcBCBorders ] [@ BCMapCellInfo::SetColumn ]
Firefox: 45.0.1, Build ID: 20160315153207
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0

Hi,

I have tested this issue on the latest Firefox (45.0.1) release, latest Nightly (48.0a1 - Build ID: 20160321030217) build and I have managed to reproduce it. I have tested this using the provided URL from comment 0. I have opened the page in "print preview" and tried to switch from portrait layout to landscape but the browser crashed (bp-5fff2266-3f6f-4b43-ac30-d525d2160322).

Comment 11

2 years ago
Do we know which bug# cause the regression?
Keywords: crashreportid
See Also: → bug 927267
Whiteboard: [tbird crash]
User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:49.0) Gecko/20100101 Firefox/49.0

I have tested this issue again with the latest Nightly (49.0a1, Build ID: 20160428030218) and it is no longer reproducible.
I have performed a regression range in order to find the bug that fixed the issue:

First good revision: 7b04f21506814f59eefd89808a166d593db3d53d
Last bad revision: 06a0c49f1206457a0a30138409a5e770e896f81b
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=06a0c4
9f1206457a0a30138409a5e770e896f81b&tochange=7b04f21506814f59eefd89808a166d593db3
d53d

Looks like the following bug has the changes which introduced the fix:
https://bugzilla.mozilla.org/show_bug.cgi?id=1259677
You need to log in before you can comment on or make changes to this bug.