Closed Bug 983570 Opened 6 years ago Closed 6 years ago

aState.mPrevChild is updated regardless of line->IsDirty in nsBlockFrame::ReflowDirtyLines

Categories

(Core :: Layout: Block and Inline, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: arai, Assigned: arai)

References

Details

(Keywords: perf, regression)

Attachments

(2 files, 1 obsolete file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release)
Build ID: 20140306171728

Steps to reproduce:

In nsBlockFrame.cpp,
http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsBlockFrame.cpp#2007
>       if (line->IsDirty())
>         NS_ASSERTION(line->mFirstChild->GetPrevSibling() ==
>                      line.prev()->LastChild(), "unexpected line frames");
>         aState.mPrevChild = line->mFirstChild->GetPrevSibling();

Here, aState.mPrevChild is updated regardless of if-condition.

Which was updated in following patch
http://hg.mozilla.org/mozilla-central/diff/aaed6b9c8952/layout/generic/nsBlockFrame.cpp
and the previous code was following:
>       if (line->IsDirty())
>         aState.mPrevChild = line.prev()->LastChild();

So I think that the code should be following:
>       if (line->IsDirty()) {
>         NS_ASSERTION(line->mFirstChild->GetPrevSibling() ==
>                      line.prev()->LastChild(), "unexpected line frames");
>         aState.mPrevChild = line->mFirstChild->GetPrevSibling();
>       }

Could anyone verify it?
OS: Mac OS X → All
Hardware: x86 → All
Yes, it's a mistake and your suggestion is correct.  Do you want to write a patch to fix it?
Keywords: perf
Priority: -- → P4
Attached patch Add braces after if-condition (obsolete) — Splinter Review
Thank you for verifying!
Here is a patch.
Attachment #8391120 - Flags: review?(matspal)
Comment on attachment 8391120 [details] [diff] [review]
Add braces after if-condition

Please change the commit message so that it describes the patch
rather than the bug.  For example "Update aState.mPrevChild only when
line->IsDirty ..." or "Add missing braces ..." or some such.
Also add r=mats at the end, to follow the convention.
Thanks!
Attachment #8391120 - Flags: review?(matspal) → review+
Thank you!
Could you push this patch to try server?
Attachment #8391120 - Attachment is obsolete: true
Certainly.  (I limited it to one platform to save resources since I don't expect
any platform parity for this change.)
https://tbpl.mozilla.org/?tree=Try&rev=ab0a5be46e35
Attachment #8391259 - Flags: review+
Just to satisfy my curiosity - how did you find this bug?  did you just stumble upon it
when reading the code, or did you use some analysis tool?
Thank you again!

I used this script to find suspect code (control statement without braces, followed by 2 same indent statements),
and verified each of them manually.
this script is very buggy, and most of its output are not bug :)
Cool. Nice catch!  The Try run is green btw, so this is ready to land.
Assignee: nobody → arai_a
Blocks: 728255
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/5a675f0c49fb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.