Closed Bug 1222783 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect, critical)

42 Branch
x86
Windows NT
defect
Not set
critical

Tracking

()

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

People

(Reporter: philipp, Assigned: dbaron)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(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 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) firefox@browser-security.de 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:
isec@securitascout.com, Version 4
sys@foxysecurity.com, Version 4
fx@foxysecureKDJJHVLSDUVFU.com, Version 6
fox@foxy.sec.com, Version 6
zz@JDkfjdK, Version 6
sys@foxysecurity.com, Version 4
security@protegere.org, 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.
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.
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
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)
https://hg.mozilla.org/integration/mozilla-inbound/rev/76753a9a19fd1043002a4ff8f5babe2682a97eb2
Bug 1222783 - Make nsHTMLFramesetFrame::Reflow set firstTime based on what firstTime means.  r=roc
https://hg.mozilla.org/mozilla-central/rev/76753a9a19fd
Status: NEW → RESOLVED
Closed: 4 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.