1329090 - Use invalid size in GrResourceProvider::createBuffer in firefox 51

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Kevin Chen[:kechen] (UTC + 8)]

In Bug 1306628, we uplifted a patch[1] to beta. However, the skia in beta is in m51 and do not use the variable allocSize which causes the size we use to allocate the buffer might smaller than MIN_SIZE.

[1] https://hg.mozilla.org/releases/mozilla-beta/diff/057879e7de42/gfx/skia/skia/src/gpu/GrResourceProvider.cpp

Comment 1

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Fix the misuse of the variable

Hello Lee, Should we fix this ?

Comment 2

[Lee Salzman [:lsalzman]]

Comment on attachment 8824336 [details] [diff] [review]
Fix the misuse of the variable

Okay, yes, go ahead and commit this for beta. It appears that there was a difference in the Skia code between beta and nightly/aurora that got overlooked during the backport. But this patch would fix it.

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

There some test fails on try run, I will check if they are related to this fix.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3603b3c7f385d5a3e1d33f54dc4bc11e6e6508d1

Comment 4

[Kevin Chen[:kechen] (UTC + 8)]

The try failures in comment 3 are not really related to this fix, some of the tests got failed in pure beta[1].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ee2804c1f6dd320dbf0884c187d432fb6be57bf

However, since the next release day is a little bit close(2017/1/23) and I am not really familiar with this part of code, Lee, can you help me to evaluate the risk for beta uplifting ?

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1306628
[User impact if declined]:
Without this patch GrResourceProvider::createBuffer cannot handle large size buffer creation.
[Is this code covered by automated tests?]:
None
[Has the fix been verified in Nightly?]:
Need to figure out.
[Needs manual test from QE? If yes, steps to reproduce]: 
Need to figure out.
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
Need to figure out.
[Why is the change risky/not risky?]:
Need to figure out.
[String changes made/needed]:
None

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #4)
> The try failures in comment 3 are not really related to this fix, some of
> the tests got failed in pure beta[1].
> 
> [1]
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=6ee2804c1f6dd320dbf0884c187d432fb6be57bf
> 
> However, since the next release day is a little bit close(2017/1/23) and I
> am not really familiar with this part of code, Lee, can you help me to
> evaluate the risk for beta uplifting ?
> 
> Approval Request Comment
> [Feature/Bug causing the regression]:
> Bug 1306628
> [User impact if declined]:
> Without this patch GrResourceProvider::createBuffer cannot handle large size
> buffer creation.
> [Is this code covered by automated tests?]:
> None
> [Has the fix been verified in Nightly?]:
> Need to figure out.
> [Needs manual test from QE? If yes, steps to reproduce]: 
> Need to figure out.
> [List of other uplifts needed for the feature/fix]:
> None
> [Is the change risky?]:
> Need to figure out.
> [Why is the change risky/not risky?]:
> Need to figure out.
> [String changes made/needed]:
> None

The change is not risky. It just fixes a patch that to work as expected. We already used this patch in central and aurora without any problems, and similar Skia upstream changes already exist too.

Comment 6

[Kevin Chen[:kechen] (UTC + 8)]

Comment on attachment 8824336 [details] [diff] [review]
Fix the misuse of the variable

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1306628
[User impact if declined]:
Without this patch GrResourceProvider::createBuffer cannot handle large size buffer creation.
[Is this code covered by automated tests?]:
None
[Has the fix been verified in Nightly?]:
Yes, and uplifted to aurora too.
[Needs manual test from QE? If yes, steps to reproduce]: 
No
[List of other uplifts needed for the feature/fix]:
None
[Is the change risky?]:
Not risky.
[Why is the change risky/not risky?]:
We've already had this fix in nightly and aurora without problem and also a upstream for skia.
[String changes made/needed]:

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8824336 [details] [diff] [review]
Fix the misuse of the variable

Additional fix for beta only to go along with bug 1306628. 
This should make it into monday's 51 RC build.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/251731790dc1