Closed Bug 1279354 Opened 3 years ago Closed 3 years ago

Null deref crash in nsProgressFrame::ReflowBarFrame

Categories

(Core :: Layout: Form Controls, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: mccr8, Assigned: mats)

References

()

Details

(Keywords: crash, testcase)

Crash Data

Attachments

(4 files)

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.
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
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
STR: Print Preview any product page at http://www.unieuro.it/ for example:
http://www.unieuro.it/online/Notebook/15-ba064nl-pidHEW15BA064NL
Assignee: nobody → mats
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.
Keywords: testcase
OS: Linux → All
Hardware: Unspecified → All
(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.)
Attachment #8814968 - Flags: review?(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
Attachment #8814968 - Flags: review?(dholbert) → review+
(Do be sure to replace the "r=bz" in the commit message from part 1)
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.)
Attachment #8814969 - Flags: review?(dholbert) → review+
Pushed by mpalmgren@mozilla.com:
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.
Flags: in-testsuite+
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.
Attachment #8814968 - Flags: approval-mozilla-beta?
Attachment #8814968 - Flags: approval-mozilla-aurora?
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.
Attachment #8814968 - Flags: approval-mozilla-beta?
Attachment #8814968 - Flags: approval-mozilla-beta+
Attachment #8814968 - Flags: approval-mozilla-aurora?
Attachment #8814968 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.