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

Status: RESOLVED FIXED

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

Description

[Tooru Fujisawa [:arai]]

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?

Comment 1

[Mats Palmgren (inactive)]

Yes, it's a mistake and your suggestion is correct.  Do you want to write a patch to fix it?

Comment 2

[Tooru Fujisawa [:arai]]

Attachment: Add braces after if-condition

Thank you for verifying!
Here is a patch.

Comment 3

[Mats Palmgren (inactive)]

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!

Comment 4

[Tooru Fujisawa [:arai]]

Attachment: addressing review comments

Thank you!
Could you push this patch to try server?

Comment 5

[Mats Palmgren (inactive)]

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

Comment 6

[Mats Palmgren (inactive)]

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?

Comment 7

[Tooru Fujisawa [:arai]]

Attachment: Script to find suspect statement

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 :)

Comment 8

[Mats Palmgren (inactive)]

Cool. Nice catch!  The Try run is green btw, so this is ready to land.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a675f0c49fb

Comment 10

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/5a675f0c49fb