1222783 - crash in nsHTMLFramesetFrame::Reflow in Firefox 42 with particular addons present

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-52c3a6d7-e247-4457-9a36-d24d92151105.
=============================================================
Crashing Thread
Frame 	Module 	Signature 	Source
0 	xul.dll 	nsHTMLFramesetFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) 	layout/generic/nsFrameSetFrame.cpp
1 	xul.dll 	nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, nsHTMLReflowState&, unsigned int&, nsBlockReflowState&) 	layout/generic/nsBlockReflowContext.cpp
2 	xul.dll 	nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp
3 	xul.dll 	nsBlockFrame::ReflowLine(nsBlockReflowState&, nsLineList_iterator, bool*) 	layout/generic/nsBlockFrame.cpp

this nsHTMLFramesetFrame::Reflow crash signature is regressing in firefox 42 and currently on #11 of the top score board. 
numerous user comments say, this is happening while trying to log into the ing-diba.de (german bank) website, subsequently over 95% of the crashes happen on de locale builds. some other urls are also mentioned

looking at correlation data for this signature, it strongly correlates with a particular extension:
68% (69/101) vs. 0% (87/44192) [e-mail] https://addons.mozilla.org/firefox/addon/browser-security-1/
and there are also another bunch of obscure security-labelled extensions which seem involved but only exist on a handful of devices overall:
[e-mail], Version 4
[e-mail], Version 4
[e-mail], Version 6
[e-mail], Version 6
zz@JDkfjdK, Version 6
[e-mail], Version 4
[e-mail], Version 3

the crash is reproducible:
1. install the addon from https://addons.mozilla.org/firefox/addon/browser-security-1/
2. visit https://banking.ing-diba.de/app/login, enter some random login data into the form, let the firefox pw manager save it & restart the browser.
3. visit https://banking.ing-diba.de/app/login again. in most cases firefox now crashes on the spot - sometimes it also takes another time. go the homepage https://www.ing-diba.de/ and click on the orange "Log-in Banking" button on the top right for the crash

regression testing the issue narrows the window down to the changeset https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=40fb01ba8c9287c5fb982a8cbffdad6862c296fc&tochange=5dcb38c7f1b84d7fc4d0255a2b10165dfaceb941 so i'm also cc'ing dbaron on this bug.

Comment 1

[Robert Kaiser]

reflow and dbaron's patches are layout, so moving into that component.

Comment 2

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

Null dereference of mChildBorderColors, presumably because we call nsHTMLFramesetFrame::Reflow twice while NS_FRAME_FIRST_REFLOW is set.  I think that's an ok thing to do, and we should just define the firstTime variable in a better way.

Comment 3

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

I didn't actually test the steps-to-reproduce since they're complicated, and the crash was clear enough from the crash stack.

I have a simple testcase (written based on knowing the regressing bug and the stack) that crashes in current builds, and a patch that I believe should fix it that I'll test tomorrow:
https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/81fdd3806ce9/frameset-set-firsttime-better

Comment 4

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

Attachment: Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means

I confirmed locally that the new crashtest crashes in the harness
without the patch, and passes in the harness with the patch.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/76753a9a19fd1043002a4ff8f5babe2682a97eb2
Bug 1222783 - Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means.  r=roc

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/76753a9a19fd

Comment 7

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

Would you be willing to retest the steps from comment 0 in a nightly from November 12th (which should be out within a few hours) to confirm that this is fixed?

Comment 8

[[:philipp]]

sure, i can confirm that in today's nightly the str from comment #0 don't produce this crash any longer.

Comment 9

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

Comment on attachment 8684912 [details] [diff] [review]
Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means

Approval Request Comment
[Feature/regressing bug #]: bug 538194 (didn't introduce the bug, but made an existing bug show up more)
[User impact if declined]: topcrash #11, involving interaction between extensions and certain pages
[Describe test coverage new/current, TreeHerder]: crashtest in tree; manually tested in comment 8
[Risks and why]: pretty low risk; it's just using a more reliable test to make sure we only execute certain code once per object
[String/UUID change made/needed]: no

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8684912 [details] [diff] [review]
Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means

Given that the fix was verified on Nightly and crash fixes are so important, I feel comfortable taking this in Aurora44.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8684912 [details] [diff] [review]
Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means

Crash fix, recent regression (or worsening of an existing one) 
Let's take this in beta.

Comment 12

[Carsten Book [:Tomcat]]

this failed to apply to aurora:

grafting 314187:76753a9a19fd "Bug 1222783 - Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means.  r=roc"
merging layout/generic/crashtests/crashtests.list
warning: conflicts during merge.
merging layout/generic/crashtests/crashtests.list incomplete! (edit conflicts, then use 'hg resolve --mark')
merging layout/generic/nsFrameSetFrame.cpp
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)

Comment 13

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/eaf685ae45f1

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/7947f1e4ca76

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/eaf685ae45f1