Closed Bug 1222783 Opened 6 years ago Closed 6 years ago

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


(Core :: Layout, defect)

42 Branch
Windows NT
Not set



Tracking Status
firefox41 --- unaffected
firefox42 --- affected
firefox43 --- fixed
firefox44 --- fixed
firefox45 --- verified
b2g-v2.5 --- fixed


(Reporter: philipp, Assigned: dbaron)



(Keywords: crash, regression)

Crash Data


(1 file)

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 (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)
and there are also another bunch of obscure security-labelled extensions which seem involved but only exist on a handful of devices overall:, Version 4, Version 4, Version 6, Version 6
zz@JDkfjdK, Version 6, Version 4, Version 3

the crash is reproducible:
1. install the addon from
2. visit, enter some random login data into the form, let the firefox pw manager save it & restart the browser.
3. visit again. in most cases firefox now crashes on the spot - sometimes it also takes another time. go the homepage 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 so i'm also cc'ing dbaron on this bug.
reflow and dbaron's patches are layout, so moving into that component.
Component: General → Layout
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.
Blocks: 538194
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:
Assignee: nobody → dbaron
I confirmed locally that the new crashtest crashes in the harness
without the patch, and passes in the harness with the patch.
Attachment #8684912 - Flags: review?(roc)
Bug 1222783 - Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means.  r=roc
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
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?
Flags: needinfo?(madperson)
sure, i can confirm that in today's nightly the str from comment #0 don't produce this crash any longer.
Flags: needinfo?(madperson)
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
Attachment #8684912 - Flags: approval-mozilla-beta?
Attachment #8684912 - Flags: approval-mozilla-aurora?
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.
Attachment #8684912 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8684912 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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)
Flags: needinfo?(dbaron)
You need to log in before you can comment on or make changes to this bug.