Remove unnecessary decoded frame size overflow check

Remove unnecessary decoded frame size overflow check

This patch removes unnecessary decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None

Review-Url: https://codereview.chromium.org/2907143002
Cr-Commit-Position: refs/heads/master@{#479772}
Committed: https://chromium.googlesource.com/chromium/src/+/ffb49300060096adc5d1d49594a60fd69c821a64

[naga, 2017-05-29 07:24:10]

Description was changed from

==========
Remove unwanted decoded frame size overflow check.

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
==========

to

==========
Remove unwanted decoded frame size overflow check.

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
R=SamsungPeerReview
==========

[naga, 2017-05-29 07:24:10]

nagarajan.n@samsung.com changed reviewers:
+ shanmuga.m@samsung.com, srirama.m@samsung.com

[naga, 2017-05-29 13:03:11]

Description was changed from

==========
Remove unwanted decoded frame size overflow check.

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
R=SamsungPeerReview
==========

to

==========
Remove unwanted decoded frame size overflow check.

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
==========

[naga, 2017-05-29 13:03:11]

nagarajan.n@samsung.com changed reviewers:
+ noel@chromium.org, scroggo@chromium.org

[Srirama, 2017-05-30 07:07:39]

https://codereview.chromium.org/2907143002/diff/1/third_party/WebKit/Source/p...
File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp (left):

https://codereview.chromium.org/2907143002/diff/1/third_party/WebKit/Source/p...
third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:412: if
(frame_memory_usage / 4 != frame_area) {          // overflow occurred
though it looks like it is doing nothing, this would have been added to detect
overflow. Please check the history of the CL why it was added to get a better
understanding. If it needs to be removed probably we need to remove the below
part of the code as well.
Anyway i will leave it to scroggo@ and noel@ to comment on this.

[Noel Gordon, 2017-05-30 11:40:22]

Yes please look the changes that added this code.  Sure looks to me like integer
overflow was a concern, based on the comments in the current code.

[naga, 2017-05-30 11:43:13]

On 2017/05/30 11:40:22, noel gordon wrote:
> Yes please look the changes that added this code.  Sure looks to me like
integer
> overflow was a concern, based on the comments in the current code.

Before decoding start we had this condition check in
SizeCalculationMayOverflow(). 
If overflow occurs decoding will not start. so this condition check is not
required here.

[naga, 2017-05-30 12:07:40]

nagarajan.n@samsung.com changed reviewers:
+ cblume@chromium.org

[naga, 2017-05-30 12:09:55]

On 2017/05/30 07:07:39, Srirama wrote:
>
https://codereview.chromium.org/2907143002/diff/1/third_party/WebKit/Source/p...
> File third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp
(left):
> 
>
https://codereview.chromium.org/2907143002/diff/1/third_party/WebKit/Source/p...
> third_party/WebKit/Source/platform/image-decoders/ImageDecoder.cpp:412: if
> (frame_memory_usage / 4 != frame_area) {          // overflow occurred
> though it looks like it is doing nothing, this would have been added to detect
> overflow. Please check the history of the CL why it was added to get a better
> understanding. If it needs to be removed probably we need to remove the below
> part of the code as well.
> Anyway i will leave it to scroggo@ and noel@ to comment on this.

1st condition is not required because it's already taken care while setting the
decoded size.
Second condition is required because it depends number of previous frame size.

[cblume, 2017-05-30 18:22:47]

Hello naga,

I am the person who originally wrote this code. You can see the original code
review here [1].

SizeCalculationMayOverflow() is only called in ImageDecoder::SetSize().
The purpose of SizeCalculationMayOverflow() is to find if the image width *
height * 4 is larger what can be represented in a signed 32-bit int.

If I understand correctly, all of the image decoders will parse the size and
call SetSize() before doing anything with an individual frame. And if the size
overflows, we call SetFailed() and return early.


So I think you might be right that we can never get to this point and
potentially overflow.


And you are also right that the following overflow check is for multiple frames
and is still needed.

I believe your patch is correct. I considered asking for a comment explaining
why we do not need to check for overflow there, but I think that would not
provide much value to future developers. So I like this patch as-is.

Non-owner LGTM


[1] https://codereview.chromium.org/2376033005

[scroggo_chromium, 2017-05-30 19:27:59]

On 2017/05/30 18:22:47, cblume wrote:
> Hello naga,
> 
> I am the person who originally wrote this code. You can see the original code
> review here [1].
> 
> SizeCalculationMayOverflow() is only called in ImageDecoder::SetSize().
> The purpose of SizeCalculationMayOverflow() is to find if the image width *
> height * 4 is larger what can be represented in a signed 32-bit int.
> 
> If I understand correctly, all of the image decoders will parse the size and
> call SetSize() before doing anything with an individual frame. And if the size
> overflows, we call SetFailed() and return early.
> 
> 
> So I think you might be right that we can never get to this point and
> potentially overflow.
> 
> 
> And you are also right that the following overflow check is for multiple
frames
> and is still needed.
> 
> I believe your patch is correct. I considered asking for a comment explaining
> why we do not need to check for overflow there, but I think that would not
> provide much value to future developers. So I like this patch as-is.

Perhaps a DCHECK is the right way to go here?

DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());

> 
> Non-owner LGTM
> 
> 
> [1] https://codereview.chromium.org/2376033005

[naga, 2017-05-31 03:15:40]

On 2017/05/30 19:27:59, scroggo_chromium wrote:
> On 2017/05/30 18:22:47, cblume wrote:
> > Hello naga,
> > 
> > I am the person who originally wrote this code. You can see the original
code
> > review here [1].
> > 
> > SizeCalculationMayOverflow() is only called in ImageDecoder::SetSize().
> > The purpose of SizeCalculationMayOverflow() is to find if the image width *
> > height * 4 is larger what can be represented in a signed 32-bit int.
> > 
> > If I understand correctly, all of the image decoders will parse the size and
> > call SetSize() before doing anything with an individual frame. And if the
size
> > overflows, we call SetFailed() and return early.
> > 
> > 
> > So I think you might be right that we can never get to this point and
> > potentially overflow.
> > 
> > 
> > And you are also right that the following overflow check is for multiple
> frames
> > and is still needed.
> > 
> > I believe your patch is correct. I considered asking for a comment
explaining
> > why we do not need to check for overflow there, but I think that would not
> > provide much value to future developers. So I like this patch as-is.
> 
> Perhaps a DCHECK is the right way to go here?
> 
> DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> 
> > 
> > Non-owner LGTM
> > 
> > 
> > [1] https://codereview.chromium.org/2376033005

I feel DCHECK is not required here

[naga, 2017-05-31 03:31:56]

On 2017/05/31 03:15:40, naga wrote:
> On 2017/05/30 19:27:59, scroggo_chromium wrote:
> > On 2017/05/30 18:22:47, cblume wrote:
> > > Hello naga,
> > > 
> > > I am the person who originally wrote this code. You can see the original
> code
> > > review here [1].
> > > 
> > > SizeCalculationMayOverflow() is only called in ImageDecoder::SetSize().
> > > The purpose of SizeCalculationMayOverflow() is to find if the image width
*
> > > height * 4 is larger what can be represented in a signed 32-bit int.
> > > 
> > > If I understand correctly, all of the image decoders will parse the size
and
> > > call SetSize() before doing anything with an individual frame. And if the
> size
> > > overflows, we call SetFailed() and return early.
> > > 
> > > 
> > > So I think you might be right that we can never get to this point and
> > > potentially overflow.
> > > 
> > > 
> > > And you are also right that the following overflow check is for multiple
> > frames
> > > and is still needed.
> > > 
> > > I believe your patch is correct. I considered asking for a comment
> explaining
> > > why we do not need to check for overflow there, but I think that would not
> > > provide much value to future developers. So I like this patch as-is.
> > 
> > Perhaps a DCHECK is the right way to go here?
> > 
> > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > 
> > > 
> > > Non-owner LGTM
> > > 
> > > 
> > > [1] https://codereview.chromium.org/2376033005
> 
> I feel DCHECK is not required here

Added DCHECK for total_memory_usage

[Noel Gordon, 2017-05-31 04:23:28]

On 2017/05/31 03:31:56, naga wrote:
> On 2017/05/31 03:15:40, naga wrote:
> > On 2017/05/30 19:27:59, scroggo_chromium wrote:
> > > On 2017/05/30 18:22:47, cblume wrote:
> > > > Hello naga,
> > > > 
> > > > I am the person who originally wrote this code. You can see the original
> > code
> > > > review here [1].
> > > > 
> > > > SizeCalculationMayOverflow() is only called in ImageDecoder::SetSize().
> > > > The purpose of SizeCalculationMayOverflow() is to find if the image
width
> *
> > > > height * 4 is larger what can be represented in a signed 32-bit int.
> > > > 
> > > > If I understand correctly, all of the image decoders will parse the size
> and
> > > > call SetSize() before doing anything with an individual frame. And if
the
> > size
> > > > overflows, we call SetFailed() and return early.
> > > > 
> > > > 
> > > > So I think you might be right that we can never get to this point and
> > > > potentially overflow.
> > > > 
> > > > 
> > > > And you are also right that the following overflow check is for multiple
> > > frames
> > > > and is still needed.
> > > > 
> > > > I believe your patch is correct. I considered asking for a comment
> > explaining
> > > > why we do not need to check for overflow there, but I think that would
not
> > > > provide much value to future developers. So I like this patch as-is.
> > > 
> > > Perhaps a DCHECK is the right way to go here?
> > > 
> > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > 
> > > > 
> > > > Non-owner LGTM
> > > > 
> > > > 
> > > > [1] https://codereview.chromium.org/2376033005
> > 
> > I feel DCHECK is not required here
> 
> Added DCHECK for total_memory_usage

Good.

[Noel Gordon, 2017-05-31 04:49:00]

On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > provide much value to future developers. So I like this patch as-is.
> > > > 
> > > > Perhaps a DCHECK is the right way to go here?
> > > > 
> > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > 
> > > > > 
> > > > > Non-owner LGTM
> > > > > 
> > > > > 
> > > > > [1] https://codereview.chromium.org/2376033005
> > > 
> > > I feel DCHECK is not required here
> > 
> > Added DCHECK for total_memory_usage
> 
> Good.

On second thoughts, a DCHECK might be a good idea if overflow can potentially
happen in field. 

> And you are also right that the following overflow check is for multiple
frames
and is still needed.

Agree. The second block of code dealing with overflow should say, right?

Alsp,  a change description should explain "why" we are making this change, not
'what" was changed.  E.g., explain why it's safe to remove the first overflow
check,  and so on.  This so, years hence, we have an explanation of the change
that will still makes sense.

[Noel Gordon, 2017-05-31 04:49:58]

On 2017/05/31 04:49:00, noel gordon wrote:
> On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > provide much value to future developers. So I like this patch as-is.
> > > > > 
> > > > > Perhaps a DCHECK is the right way to go here?
> > > > > 
> > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > 
> > > > > > 
> > > > > > Non-owner LGTM
> > > > > > 
> > > > > > 
> > > > > > [1] https://codereview.chromium.org/2376033005
> > > > 
> > > > I feel DCHECK is not required here
> > > 
> > > Added DCHECK for total_memory_usage
> > 
> > Good.
> 
> On second thoughts, a DCHECK might be a good idea if overflow can potentially
> happen in field. 

Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if overflow can
potentially
happen in field.

[naga, 2017-06-01 03:47:19]

On 2017/05/31 04:49:58, noel gordon wrote:
> On 2017/05/31 04:49:00, noel gordon wrote:
> > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > provide much value to future developers. So I like this patch
as-is.
> > > > > > 
> > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > 
> > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > 
> > > > > > > 
> > > > > > > Non-owner LGTM
> > > > > > > 
> > > > > > > 
> > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > 
> > > > > I feel DCHECK is not required here
> > > > 
> > > > Added DCHECK for total_memory_usage
> > > 
> > > Good.
> > 
> > On second thoughts, a DCHECK might be a good idea if overflow can
potentially
> > happen in field. 
> 
> Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if overflow can
> potentially
> happen in field.

[naga, 2017-06-01 03:47:21]

On 2017/05/31 04:49:58, noel gordon wrote:
> On 2017/05/31 04:49:00, noel gordon wrote:
> > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > provide much value to future developers. So I like this patch
as-is.
> > > > > > 
> > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > 
> > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > 
> > > > > > > 
> > > > > > > Non-owner LGTM
> > > > > > > 
> > > > > > > 
> > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > 
> > > > > I feel DCHECK is not required here
> > > > 
> > > > Added DCHECK for total_memory_usage
> > > 
> > > Good.
> > 
> > On second thoughts, a DCHECK might be a good idea if overflow can
potentially
> > happen in field. 
> 
> Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if overflow can
> potentially
> happen in field.

yes it's required I will revert the change made in patch 2

[naga, 2017-06-07 14:13:00]

Patchset #2 (id:20001) has been deleted

[naga, 2017-06-07 14:13:40]

On 2017/06/01 03:47:21, naga wrote:
> On 2017/05/31 04:49:58, noel gordon wrote:
> > On 2017/05/31 04:49:00, noel gordon wrote:
> > > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > > provide much value to future developers. So I like this patch
> as-is.
> > > > > > > 
> > > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > > 
> > > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > > 
> > > > > > > > 
> > > > > > > > Non-owner LGTM
> > > > > > > > 
> > > > > > > > 
> > > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > > 
> > > > > > I feel DCHECK is not required here
> > > > > 
> > > > > Added DCHECK for total_memory_usage
> > > > 
> > > > Good.
> > > 
> > > On second thoughts, a DCHECK might be a good idea if overflow can
> potentially
> > > happen in field. 
> > 
> > Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if overflow
can
> > potentially
> > happen in field.
> 
> yes it's required I will revert the change made in patch 2

Deleted the patch set #2. Patch set #1 looks good

[Noel Gordon, 2017-06-07 16:02:36]

On 2017/06/07 14:13:40, naga wrote:
> On 2017/06/01 03:47:21, naga wrote:
> > On 2017/05/31 04:49:58, noel gordon wrote:
> > > On 2017/05/31 04:49:00, noel gordon wrote:
> > > > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > > > provide much value to future developers. So I like this patch
> > as-is.
> > > > > > > > 
> > > > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > > > 
> > > > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > Non-owner LGTM
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > > > 
> > > > > > > I feel DCHECK is not required here
> > > > > > 
> > > > > > Added DCHECK for total_memory_usage
> > > > > 
> > > > > Good.
> > > > 
> > > > On second thoughts, a DCHECK might be a good idea if overflow can
> > potentially
> > > > happen in field. 
> > > 
> > > Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if overflow
> can
> > > potentially
> > > happen in field.
> > 
> > yes it's required I will revert the change made in patch 2
> 
> Deleted the patch set #2. Patch set #1 looks good

First, and for future reference, no need to delete patches, just upload a new
patch :)

Second, let me rephrase my question.  An overflow was removed in this CL.  Can
that overflow potentially happen in field?   Anyone can answer.

[naga, 2017-06-13 08:37:30]

On 2017/06/07 16:02:36, noel gordon wrote:
> On 2017/06/07 14:13:40, naga wrote:
> > On 2017/06/01 03:47:21, naga wrote:
> > > On 2017/05/31 04:49:58, noel gordon wrote:
> > > > On 2017/05/31 04:49:00, noel gordon wrote:
> > > > > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > > > > provide much value to future developers. So I like this
patch
> > > as-is.
> > > > > > > > > 
> > > > > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > > > > 
> > > > > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > Non-owner LGTM
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > > > > 
> > > > > > > > I feel DCHECK is not required here
> > > > > > > 
> > > > > > > Added DCHECK for total_memory_usage
> > > > > > 
> > > > > > Good.
> > > > > 
> > > > > On second thoughts, a DCHECK might be a good idea if overflow can
> > > potentially
> > > > > happen in field. 
> > > > 
> > > > Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if
overflow
> > can
> > > > potentially
> > > > happen in field.
> > > 
> > > yes it's required I will revert the change made in patch 2
> > 
> > Deleted the patch set #2. Patch set #1 looks good
> 
> First, and for future reference, no need to delete patches, just upload a new
> patch :)
> 
> Second, let me rephrase my question.  An overflow was removed in this CL.  Can
> that overflow potentially happen in field?   Anyone can answer.

I feel this overflow mostly will not occur in the field

[scroggo_chromium, 2017-06-13 15:01:36]

On 2017/06/13 08:37:30, naga wrote:
> On 2017/06/07 16:02:36, noel gordon wrote:
> > On 2017/06/07 14:13:40, naga wrote:
> > > On 2017/06/01 03:47:21, naga wrote:
> > > > On 2017/05/31 04:49:58, noel gordon wrote:
> > > > > On 2017/05/31 04:49:00, noel gordon wrote:
> > > > > > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > > > > > provide much value to future developers. So I like this
> patch
> > > > as-is.
> > > > > > > > > > 
> > > > > > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > > > > > 
> > > > > > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Non-owner LGTM
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > > > > > 
> > > > > > > > > I feel DCHECK is not required here
> > > > > > > > 
> > > > > > > > Added DCHECK for total_memory_usage
> > > > > > > 
> > > > > > > Good.
> > > > > > 
> > > > > > On second thoughts, a DCHECK might be a good idea if overflow can
> > > > potentially
> > > > > > happen in field. 
> > > > > 
> > > > > Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if
> overflow
> > > can
> > > > > potentially
> > > > > happen in field.
> > > > 
> > > > yes it's required I will revert the change made in patch 2
> > > 
> > > Deleted the patch set #2. Patch set #1 looks good
> > 
> > First, and for future reference, no need to delete patches, just upload a
new
> > patch :)
> > 
> > Second, let me rephrase my question.  An overflow was removed in this CL. 
Can
> > that overflow potentially happen in field?   Anyone can answer.
> 
> I feel this overflow mostly will not occur in the field

So it still might happen? In that case, we should address it.

[naga, 2017-06-13 16:45:37]

On 2017/06/13 15:01:36, scroggo_chromium wrote:
> On 2017/06/13 08:37:30, naga wrote:
> > On 2017/06/07 16:02:36, noel gordon wrote:
> > > On 2017/06/07 14:13:40, naga wrote:
> > > > On 2017/06/01 03:47:21, naga wrote:
> > > > > On 2017/05/31 04:49:58, noel gordon wrote:
> > > > > > On 2017/05/31 04:49:00, noel gordon wrote:
> > > > > > > On 2017/05/31 04:23:28, noel gordon wrote:
> > > > > > > > > > > > provide much value to future developers. So I like this
> > patch
> > > > > as-is.
> > > > > > > > > > > 
> > > > > > > > > > > Perhaps a DCHECK is the right way to go here?
> > > > > > > > > > > 
> > > > > > > > > > > DCHECK_EQ(frame_memory_usage / 4, DecodedSize().Area());
> > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > Non-owner LGTM
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > [1] https://codereview.chromium.org/2376033005
> > > > > > > > > > 
> > > > > > > > > > I feel DCHECK is not required here
> > > > > > > > > 
> > > > > > > > > Added DCHECK for total_memory_usage
> > > > > > > > 
> > > > > > > > Good.
> > > > > > > 
> > > > > > > On second thoughts, a DCHECK might be a good idea if overflow can
> > > > > potentially
> > > > > > > happen in field. 
> > > > > > 
> > > > > > Ahem.  On second thoughts, a DCHECK _might not_ be a good idea if
> > overflow
> > > > can
> > > > > > potentially
> > > > > > happen in field.
> > > > > 
> > > > > yes it's required I will revert the change made in patch 2
> > > > 
> > > > Deleted the patch set #2. Patch set #1 looks good
> > > 
> > > First, and for future reference, no need to delete patches, just upload a
> new
> > > patch :)
> > > 
> > > Second, let me rephrase my question.  An overflow was removed in this CL. 
> Can
> > > that overflow potentially happen in field?   Anyone can answer.
> > 
> > I feel this overflow mostly will not occur in the field
> 
> So it still might happen? In that case, we should address it.
As I mentioned the description this condition already present before decoding
the image. This patch removes the unwanted second condition check

[cblume, 2017-06-13 17:48:24]

On 2017/06/13 16:45:37, naga wrote:
> As I mentioned the description this condition already present before decoding
> the image. This patch removes the unwanted second condition check

I agree. This is what my last comment said as well.

It seems to me that all of our existing image decoders parse for the image size
and SetFailed() if that size overflows. Only after this do they begin working on
a given frame. The second overflow check cannot be reached if we call
SetFailed() because one we're in a failed state we don't try to decode future
frames.

The only real reason to keep it is if we suspect that in the future we might
change the design and allow attempting to decode a frame after SetFailed() has
been called.

[scroggo_chromium, 2017-06-13 20:37:11]

> > > > Second, let me rephrase my question.  An overflow was removed in this
CL. 
> > Can
> > > > that overflow potentially happen in field?   Anyone can answer.
> > > 
> > > I feel this overflow mostly will not occur in the field
> > 
> > So it still might happen? In that case, we should address it.
> As I mentioned the description this condition already present before decoding
> the image. This patch removes the unwanted second condition check

Ah, sorry, I misunderstood. If I understand correctly now, you mean that the
first overflow (in SizeCalculationMayOverflow) will mostly not occur in the
field (i.e. will occasionally occur in the field)?

In that case, I think this would be a perfect use a DCHECK.

I also think a description like the following sounds better:

"""
Remove unnecessary decoded frame size overflow check

Remove decoded frame size overflow check in UpdateAggressivePurging(). This
calculation
will never overflow, because SizeCalculationMayOverflow() would have returned
true and
resulted in never reaching this code.
"""

On 2017/06/13 17:48:24, cblume wrote:
> On 2017/06/13 16:45:37, naga wrote:
> > As I mentioned the description this condition already present before
decoding
> > the image. This patch removes the unwanted second condition check
> 
> I agree. This is what my last comment said as well.
> 
> It seems to me that all of our existing image decoders parse for the image
size
> and SetFailed() if that size overflows. Only after this do they begin working
on
> a given frame. The second overflow check cannot be reached if we call
> SetFailed() because one we're in a failed state we don't try to decode future
> frames.
> 
> The only real reason to keep it is if we suspect that in the future we might
> change the design and allow attempting to decode a frame after SetFailed() has
> been called.

I think future-proofing is a good reason to use a DCHECK. (i.e. The DCHECK tells
us that this cannot happen in today's code, so if you change something and it
happens, you've introduced a bug.)

That said, if the size is too big, I don't see how we could recover and decode
something (rather than what we do today, which is show the broken image icon).

[Noel Gordon, 2017-06-14 02:24:13]

On 2017/06/13 17:48:24, cblume wrote:
> On 2017/06/13 16:45:37, naga wrote:
> > As I mentioned the description this condition already present before
decoding
> > the image. This patch removes the unwanted second condition check
> 
> I agree. This is what my last comment said as well.
> 
> It seems to me that all of our existing image decoders parse for the image
size
> and SetFailed() if that size overflows. Only after this do they begin working
on
> a given frame. The second overflow check cannot be reached if we call
> SetFailed() because one we're in a failed state we don't try to decode future
> frames.

OK good, thanks for confirming, and so the

  First overflow check in this frame handling code
     - not needed, handled when we parse the image Size().  SGTM
  Second overflow check in this frame handling code
     - is required.  SGTM

[Noel Gordon, 2017-06-14 02:34:35]

On 2017/06/13 20:37:11, scroggo_chromium wrote:
> > > > > Second, let me rephrase my question.  An overflow was removed in this
> CL. 
> > > Can
> > > > > that overflow potentially happen in field?   Anyone can answer.
> > > > 
> > > > I feel this overflow mostly will not occur in the field
> > > 
> > > So it still might happen? In that case, we should address it.
> > As I mentioned the description this condition already present before
decoding
> > the image. This patch removes the unwanted second condition check
> 
> Ah, sorry, I misunderstood. If I understand correctly now, you mean that the
> first overflow (in SizeCalculationMayOverflow) will mostly not occur in the
> field (i.e. will occasionally occur in the field)?
> 
> In that case, I think this would be a perfect use a DCHECK.

Agree.  DCHECK is used to state the assumptions we believe is true.  As you and
Chris went on to mention, it also has some future-proofing built in.

naga: can you add the DCHECK please?

> I also think a description like the following sounds better:
> 
> """
> Remove unnecessary decoded frame size overflow check
> 
> Remove decoded frame size overflow check in UpdateAggressivePurging(). This
> calculation
> will never overflow, because SizeCalculationMayOverflow() would have returned
> true and
> resulted in never reaching this code.
> """

Nod: it better explains "why" we are making this change.

[Noel Gordon, 2017-06-14 02:35:27]

Description was changed from

==========
Remove unwanted decoded frame size overflow check.

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
==========

to

==========
Remove unwanted decoded frame size overflow check

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
==========

[naga, 2017-06-14 04:21:34]

On 2017/06/14 02:34:35, noel gordon wrote:
> On 2017/06/13 20:37:11, scroggo_chromium wrote:
> > > > > > Second, let me rephrase my question.  An overflow was removed in
this
> > CL. 
> > > > Can
> > > > > > that overflow potentially happen in field?   Anyone can answer.
> > > > > 
> > > > > I feel this overflow mostly will not occur in the field
> > > > 
> > > > So it still might happen? In that case, we should address it.
> > > As I mentioned the description this condition already present before
> decoding
> > > the image. This patch removes the unwanted second condition check
> > 
> > Ah, sorry, I misunderstood. If I understand correctly now, you mean that the
> > first overflow (in SizeCalculationMayOverflow) will mostly not occur in the
> > field (i.e. will occasionally occur in the field)?
> > 
> > In that case, I think this would be a perfect use a DCHECK.
> 
> Agree.  DCHECK is used to state the assumptions we believe is true.  As you
and
> Chris went on to mention, it also has some future-proofing built in.
> 
> naga: can you add the DCHECK please?
> 
> > I also think a description like the following sounds better:
> > 
> > """
> > Remove unnecessary decoded frame size overflow check
> > 
> > Remove decoded frame size overflow check in UpdateAggressivePurging(). This
> > calculation
> > will never overflow, because SizeCalculationMayOverflow() would have
returned
> > true and
> > resulted in never reaching this code.
> > """
> 
> Nod: it better explains "why" we are making this change.

[naga, 2017-06-14 04:22:24]

On 2017/06/14 04:21:34, naga wrote:
> On 2017/06/14 02:34:35, noel gordon wrote:
> > On 2017/06/13 20:37:11, scroggo_chromium wrote:
> > > > > > > Second, let me rephrase my question.  An overflow was removed in
> this
> > > CL. 
> > > > > Can
> > > > > > > that overflow potentially happen in field?   Anyone can answer.
> > > > > > 
> > > > > > I feel this overflow mostly will not occur in the field
> > > > > 
> > > > > So it still might happen? In that case, we should address it.
> > > > As I mentioned the description this condition already present before
> > decoding
> > > > the image. This patch removes the unwanted second condition check
> > > 
> > > Ah, sorry, I misunderstood. If I understand correctly now, you mean that
the
> > > first overflow (in SizeCalculationMayOverflow) will mostly not occur in
the
> > > field (i.e. will occasionally occur in the field)?
> > > 
> > > In that case, I think this would be a perfect use a DCHECK.
> > 
> > Agree.  DCHECK is used to state the assumptions we believe is true.  As you
> and
> > Chris went on to mention, it also has some future-proofing built in.
> > 
> > naga: can you add the DCHECK please?

I will add DCHECK 

> > 
> > > I also think a description like the following sounds better:
> > > 
> > > """
> > > Remove unnecessary decoded frame size overflow check
> > > 
> > > Remove decoded frame size overflow check in UpdateAggressivePurging().
This
> > > calculation
> > > will never overflow, because SizeCalculationMayOverflow() would have
> returned
> > > true and
> > > resulted in never reaching this code.
> > > """
> > 
> > Nod: it better explains "why" we are making this change.

ok

[naga, 2017-06-14 04:23:38]

Description was changed from

==========
Remove unwanted decoded frame size overflow check

This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). Already this condition check present
SizeCalculationMayOverflow().

BUG=None
==========

to

==========
This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow, because
SizeCalculationMayOverflow() would have returned true and resulted in never
reaching this code.
BUG=None
==========

[naga, 2017-06-14 04:24:03]

Description was changed from

==========
This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow, because
SizeCalculationMayOverflow() would have returned true and resulted in never
reaching this code.
BUG=None
==========

to

==========
This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None
==========

[scroggo_chromium, 2017-06-15 14:16:46]

> This patch removes unwanted decoded frame size overflow check in
> UpdateAggressivePurging(). This calculation will never overflow,
> because SizeCalculationMayOverflow() would have returned true and 
> resulted in never reaching this code.

You're missing a short, one-line description. Our code review tool is a little
confusing - you have the one line description as the subject, and it displays it
at the top of the page, but you also want it in your description.

My personal preference would be to use the word "unnecessary" instead of
"unwanted". In my mind, we don't want it because it's not necessary. But I won't
be a stickler about that. Add the one line description and you have my approval.

[naga, 2017-06-15 14:20:36]

Description was changed from

==========
This patch removes unwanted decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None
==========

to

==========
Remove unnecessary decoded frame size overflow check

This patch removes unnecessary decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None
==========

[naga, 2017-06-15 14:20:48]

On 2017/06/15 14:16:46, scroggo_chromium wrote:
> > This patch removes unwanted decoded frame size overflow check in
> > UpdateAggressivePurging(). This calculation will never overflow,
> > because SizeCalculationMayOverflow() would have returned true and 
> > resulted in never reaching this code.
> 
> You're missing a short, one-line description. Our code review tool is a little
> confusing - you have the one line description as the subject, and it displays
it
> at the top of the page, but you also want it in your description.
> 
> My personal preference would be to use the word "unnecessary" instead of
> "unwanted". In my mind, we don't want it because it's not necessary. But I
won't
> be a stickler about that. Add the one line description and you have my
approval.

Done

[naga, 2017-06-15 16:40:52]

Updated the description based on the comments

[scroggo_chromium, 2017-06-15 17:02:38]

On 2017/06/15 16:40:52, naga wrote:
> Updated the description based on the comments

LGTM. Thanks!

[naga, 2017-06-15 17:19:26]

The CQ bit was checked by nagarajan.n@samsung.com

[naga, 2017-06-15 17:19:26]

The patchset sent to the CQ was uploaded after l-g-t-m from cblume@chromium.org
Link to the patchset: https://codereview.chromium.org/2907143002/#ps40001
(title: "Added DCHECK and logs to check the overflow")

[commit-bot: I haz the power, 2017-06-15 17:19:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-15 19:04:42]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497547166244570,
"parent_rev": "3a1b0a576e2ea607d09928399363284ddb602a71", "commit_rev":
"ffb49300060096adc5d1d49594a60fd69c821a64"}

[commit-bot: I haz the power, 2017-06-15 19:04:55]

Description was changed from

==========
Remove unnecessary decoded frame size overflow check

This patch removes unnecessary decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None
==========

to

==========
Remove unnecessary decoded frame size overflow check

This patch removes unnecessary decoded frame size overflow check in
UpdateAggressivePurging(). This calculation will never overflow,
because SizeCalculationMayOverflow() would have returned true and 
resulted in never reaching this code.

BUG=None

Review-Url: https://codereview.chromium.org/2907143002
Cr-Commit-Position: refs/heads/master@{#479772}
Committed:
https://chromium.googlesource.com/chromium/src/+/ffb49300060096adc5d1d49594a6...
==========

[commit-bot: I haz the power, 2017-06-15 19:04:56]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/ffb49300060096adc5d1d49594a6...