1279354 - Null deref crash in nsProgressFrame::ReflowBarFrame

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Andrew McCreight [:mccr8]]

This bug was filed from the Socorro interface and is 
report bp-905a235c-a861-4e24-a50a-645202160608.
=============================================================

It looks like aBarFrame is null here. The caller has an NS_ASSERTION() against it being null, but maybe this code should try to handle it? I only see 88 crashes across all branches, so this is not particularly common.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsProgressFrame::ReflowBarFrame':
 - nightly (version 50): 5 crashes from 2016-06-06.
 - aurora  (version 49): 5 crashes from 2016-06-07.
 - beta    (version 48): 68 crashes from 2016-06-06.
 - release (version 47): 992 crashes from 2016-05-31.
 - esr     (version 45): 193 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          2          1          0          0          0          0          2
 - aurora           0          0          0          1          0          0          0
 - beta            20         15         11          5          5          5          3
 - release        171        169        161        145        129        106         38
 - esr             31         33         11         19         33         12         10

Affected platforms: Windows, Mac OS X, Linux

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsProgressFrame::ReflowBarFrame':
 - nightly (version 52): 1 crash from 2016-09-19.
 - aurora  (version 51): 1 crash from 2016-09-19.
 - beta    (version 50): 5 crashes from 2016-09-20.
 - release (version 49): 181 crashes from 2016-09-05.
 - esr     (version 45): 563 crashes from 2016-06-01.

Crash volume on the last weeks (Week N is from 10-03 to 10-09):
            W. N-1  W. N-2
 - nightly       1       0
 - aurora        1       0
 - beta          4       1
 - release     138      43
 - esr          71      57

Affected platforms: Windows, Mac OS X, Linux

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly           #996
 - aurora            #1357
 - beta    #12800    #1064
 - release #474      #305
 - esr     #155

Comment 3

[Mats Palmgren (inactive)]

STR: Print Preview any product page at http://www.unieuro.it/ for example:
http://www.unieuro.it/online/Notebook/15-ba064nl-pidHEW15BA064NL

Comment 4

[Mats Palmgren (inactive)]

Attachment: Testcase that crashes in Print Preview

Comment 5

[Mats Palmgren (inactive)]

The problem is that nsCSSFrameConstructor::ReplicateFixedFrames sets mCreatingExtraFrames = true
http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/layout/base/nsCSSFrameConstructor.cpp#9105
which makes the condition here false:
http://searchfox.org/mozilla-central/rev/9a3ab17838f039aab023837d905115f8990e442e/layout/base/nsCSSFrameConstructor.cpp#4076
(for the anon div frame, i.e. the "bar" inside the <progress>)

nsProgressFrame however use "mBarDiv->GetPrimaryFrame()" which is null in the replicated
frame subtrees.  Note that the actual frame tree is correct: the nsProgressFrame *does*
have a child frame for its anon content, it's just that we never associated the two
with a SetPrimaryFrame call.

Comment 6

[Mats Palmgren (inactive)]

Attachment: part 1 - Always call SetPrimaryFrame for NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT frames ...

(see comment 5 for an explanation of the crash)

This patch fixes the crash by itself, but I'll add a part 2 that would also
have prevented this...

(I was going to ask :bz to review this but it seems he doesn't accept review requests currently.)

Comment 7

[Mats Palmgren (inactive)]

Attachment: part 2 - Make the nsProgressFrame code a bit more idiomatic by processing its actual child frames.

Comment 8

[Mats Palmgren (inactive)]

Attachment: part 3 - crashtest

https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc84c439b622de1f0399d1b0baa8112885be788c

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8814968 [details] [diff] [review]
part 1 - Always call SetPrimaryFrame for NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT frames ...

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

r=me

Comment 10

[Daniel Holbert [:dholbert]]

(Do be sure to replace the "r=bz" in the commit message from part 1)

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8814969 [details] [diff] [review]
part 2 - Make the nsProgressFrame code a bit more idiomatic by processing its actual child frames.

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

r=me with nit below addressed, and with "r=bz" in the commit message updated to r=dholbert (same as for part 1).

::: layout/forms/nsProgressFrame.h
@@ +86,5 @@
>    virtual Element* GetPseudoElement(CSSPseudoElementType aType) override;
>  
>  protected:
> +  // Helper function to reflow a child frame.
> +  void ReflowChildFrame(nsIFrame*          aChildFrame,

s/aChildFrame/aChild/, to be consistent with how you renamed this param in the .cpp file.

(Or the reverse, if you prefer -- updating the .cpp file instead to make it consistent with this.)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ab6fa719716
part 1 - Always call SetPrimaryFrame for NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT frames (unless there is one already), including when mCreatingExtraFrames is true.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e5b80d58f43
part 2 - Make the nsProgressFrame code a bit more idiomatic by processing its actual child frames.  r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/69cad8c3ffd2
part 3 - Crashtest.

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8814968 [details] [diff] [review]
part 1 - Always call SetPrimaryFrame for NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT frames ...

Approval Request Comment
[Feature/Bug causing the regression]: I'm guessing the introduction of support for the <progress> element caused it
[User impact if declined]: crash (and it does affect users on real sites)
[Is this code covered by automated tests?] yes
[Has the fix been verified in Nightly? not yet
[Needs manual test from QE? If yes, steps to reproduce]: see comment 3 / 4
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: medium risk
[Why is the change risky/not risky?]: frame constructor changes are always a bit risky, but we're early in the cycle (I think?) so hopefully we'll discover any issues
[String changes made/needed]: none

The uplift request is for all three parts.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8ab6fa719716
https://hg.mozilla.org/mozilla-central/rev/6e5b80d58f43
https://hg.mozilla.org/mozilla-central/rev/69cad8c3ffd2

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8814968 [details] [diff] [review]
part 1 - Always call SetPrimaryFrame for NS_FRAME_ANONYMOUSCONTENTCREATOR_CONTENT frames ...

Fix a crash. Beta51+ and Aurora52+. Should be in 51 beta 6.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7e0b24a260ed
https://hg.mozilla.org/releases/mozilla-aurora/rev/c0378c5b192f
https://hg.mozilla.org/releases/mozilla-aurora/rev/ada06e30b71f

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/806eb08f9717
https://hg.mozilla.org/releases/mozilla-beta/rev/506579078765
https://hg.mozilla.org/releases/mozilla-beta/rev/3020d866784a