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

Status: RESOLVED FIXED

(Core :: Layout: Tables, defect)

Description

[Fabien Ménager]

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.

Comment 1

[Fabien Ménager]

Attachment: 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

[[:Aleksej]]

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() ]

Comment 3

[[:Aleksej]]

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

Comment 4

[Fabien Ménager]

Hello, do you think this problem can be solved in the current major version (35) ?

Comment 5

[Fabien Ménager]

May I ask you if there is something we can change in our HTML to workaround this problem quickly ?

Comment 6

[Fabien Ménager]

Any news about this ?

Comment 7

[Mats Palmgren (inactive)]

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
...

Comment 8

[Mats Palmgren (inactive)]

Attachment: 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

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

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()?

Comment 10

[Cosmin Muntean [:cmuntean], Ecosystem QA]

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

[Wayne Mery (:wsmwk)]

Do we know which bug# cause the regression?

Comment 12

[Cosmin Muntean [:cmuntean], Ecosystem QA]

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

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: fix, rebased

FWIW, here's the same patch, rebased on current trunk. (I rebased it to see if it happened to help with related-seeming bug 1442018. Sadly, it does not.)

Mats, did you intend for this to land at some point? This is r+ (though maybe a few small things to address from comment 9). It looks like this bug's crash signature is still getting hit, so I think this is still relevant.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a77e9a735527
Always call GetColFrame on the nsTableFrame's first-in-flow.  r=dbaron

Comment 15

[Mats Palmgren (inactive)]

Yeah, I think it's fine to land as is. I've double-checked
that we haven't introduced new GetColFrame calls that needs
to be addressed since then.

(Any additional cleanups can go in follow-up bugs...)

Comment 16

[Mats Palmgren (inactive)]

FTR, I also upgraded the first assertion in that method:

-  NS_ASSERTION(!GetPrevInFlow(), "GetColFrame called on next in flow");
+  MOZ_ASSERT(!GetPrevInFlow(), "GetColFrame called on next in flow");

Comment 17

[Mats Palmgren (inactive)]

This can't possibly be a regression from bug 1259677
since that change landed a year after this was reported.

Comment 18

[Wayne Mery (:wsmwk)]

Thanks for fixing it up - my late night reading misread comment 12

Comment 19

[Narcis Beleuzu [:NarcisB]]

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