898228 - Wasted work in gfxContext::ClipContainsRect()

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[pchang9]

Attachment: Suggested patch

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/20100101 Firefox/18.0 (Beta/Release)
Build ID: 20130116073211

Steps to reproduce:

The problem appears in changeset 138350:18467a85acf6. I have attached a simple one-line patch that fixes it.

In method gfxContext::ClipContainsRect() in gfx/thebes/gfxContext.cpp, the loop in line 1232 keeps overriding "lastReset" with "i" when "mStateStack[i].clipWasReset" is true. Therefore, only the last written value is visible out of the loop and all the other writes and iterations are not necessary. The patch iterates from the start of "lastReset" and breaks the first time when "i" is set.

Similar problems also appear in loops at line 2073 and 2184, function gfxContext::PushClipsToDT() and gfxContext::ChangeTransform().

Comment 1

[[:Aleksej]]

https://hg.mozilla.org/mozilla-central/file/18467a85acf6/gfx/thebes/gfxContext.cpp#l1230

Comment 2

[pchang9]

Attachment: 898228.patch

Comment 3

[Bas Schouten (:bas.schouten)]

Comment on attachment 784375 [details] [diff] [review]
898228.patch

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

Ouch! I think the current behavior is actually wrong! We're currently catching the first reset instead of the last one. This is a very nice catch, but the right solution is inserting a break in the existing loops without changing the loops itself. I'm actually surprised this hasn't caused any bugs, I suppose we don't often have multiple resets in a clip stack. When we fix this we should keep a careful eye open on whether nothing breaks.

Comment 4

[pchang9]

Attachment: 898228-1.patch

Comment 5

[Bas Schouten (:bas.schouten)]

Comment on attachment 784394 [details] [diff] [review]
898228-1.patch

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

This looks like a great change!

Comment 6

[pchang9]

(In reply to Bas Schouten (:bas.schouten) from comment #5)
> Comment on attachment 784394 [details] [diff] [review]
> 898228-1.patch
> 
> Review of attachment 784394 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like a great change!

Thanks! Could you be my voucher so that I can test on try servers ?

Comment 7

[pchang9]

Landed on try server:
https://tbpl.mozilla.org/?tree=Try&rev=75b0a15266b3

Comment 8

[pchang9]

Attachment: 898228.patch (bug description updated, try testing)

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d215826d8637

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d215826d8637