1339256 - Explicitly detect robust_buffer_access_behavior

Status: RESOLVED FIXED

(Core :: Graphics: CanvasWebGL, defect, P1)

Comment 1

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1339256 - Detect robust_buffer_access_behavior. -

Review commit: https://reviewboard.mozilla.org/r/112216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112216/

Comment 2

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1339256 - Simplify index validation. -

Review commit: https://reviewboard.mozilla.org/r/112218/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/112218/

Comment 3

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

https://reviewboard.mozilla.org/r/112216/#review113734

LGTM

Comment 4

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

https://reviewboard.mozilla.org/r/112216/#review114040

::: gfx/gl/GLContextProviderEGL.cpp:498
(Diff revision 1)
> +        contextAttribs.push_back(LOCAL_EGL_LOSE_CONTEXT_ON_RESET_EXT);
> +        contextAttribs.push_back(LOCAL_EGL_CONTEXT_OPENGL_ROBUST_ACCESS_EXT);
> +        contextAttribs.push_back(LOCAL_EGL_TRUE);
>      }
>  
> -    for (size_t i = 0; i < MOZ_ARRAY_LENGTH(gTerminationAttribs); i++) {
> +    for (const auto& cur : kTerminationAttribs) {

You replace nsTArray with std::vector is in order to use range interation here?

Comment 5

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

https://reviewboard.mozilla.org/r/112216/#review114040

> You replace nsTArray with std::vector is in order to use range interation here?

Partly, but also std::vector is the STL standard, so without a strong reason to choose nsTArray, it's easier to reach for the actual standard API.

Comment 6

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

https://reviewboard.mozilla.org/r/112218/#review114760

Looks good to me, only got one concern.

::: dom/canvas/WebGLBuffer.cpp:160
(Diff revision 1)
> +        mIndexCache = Move(newIndexCache);
> +    }
> +
> +    if (mIndexCache) {
> -    // Warning: Possibly shared memory.  See bug 1225033.
> +        // Warning: Possibly shared memory.  See bug 1225033.
> -    if (!ElementArrayCacheBufferData(data, size)) {
> +        memcpy(mIndexCache.get(), data, size);

What happens if `newIndexCache == null && mIndexCache != null && sizeChanges==false && !(target == LOCAL_GL_ELEMENT_ARRAY_BUFFER && mContext->mNeedsIndexValidation)` ?

if this is impossible, let's have an assert or something indicating it. If possible, then we'd do a memcpy into a wrongly-sized buffer here.

Comment 7

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/1-2/

Comment 8

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

https://reviewboard.mozilla.org/r/112218/#review114760

> What happens if `newIndexCache == null && mIndexCache != null && sizeChanges==false && !(target == LOCAL_GL_ELEMENT_ARRAY_BUFFER && mContext->mNeedsIndexValidation)` ?
> 
> if this is impossible, let's have an assert or something indicating it. If possible, then we'd do a memcpy into a wrongly-sized buffer here.

I rearranged the code to be more clear about what happens when.
Also we always take a copy when needed to avoid validation races with SharedArrayBuffer.

Comment 9

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/2-3/

Comment 10

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Non-trivial changes, but can't seem to revert the r+ to r? in reviewboard.

Comment 11

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

https://reviewboard.mozilla.org/r/112218/#review115190

Thanks, looks good to me.

::: dom/canvas/gtest/moz.build
(Diff revision 3)
>  
>  with Files('**'):
>      BUG_COMPONENT = ('Core', 'Canvas: 2D')
>  
> -with Files('*WebGL*'):
> -    BUG_COMPONENT = ('Core', 'Canvas: WebGL')

why are we removing this bug component specification?

Comment 12

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

https://reviewboard.mozilla.org/r/112218/#review115190

> why are we removing this bug component specification?

There's actually a linter which checks if we assign a BUG_COMPONENT to zero files, and errors about leaving this here.

Comment 13

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/3-4/

Comment 14

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

More review.
Previous versions didn't support BufferSubData updates!

Comment 15

[Dzmitry Malyshau [:kvark]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

https://reviewboard.mozilla.org/r/112218/#review116736

LGTM

Comment 16

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1339256 - Double-check robustness. -

Review commit: https://reviewboard.mozilla.org/r/118016/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118016/

Comment 17

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112216/diff/1-2/

Comment 18

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/4-5/

Comment 19

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8844704 [details]
Bug 1339256 - Double-check robustness. -

https://reviewboard.mozilla.org/r/118016/#review119848

r=me after fixing the nit of redundant if-scope.

::: gfx/gl/GLContext.cpp:1162
(Diff revision 1)
> +            NS_WARNING("Robustness supported, but not active!");
> +            MarkUnsupported(GLFeature::robustness);
> +        }
> +    }
> +    if (IsSupported(GLFeature::robustness)) {
>          const SymLoadStruct symbols[] = {

You might have a mistake on writing a redundant scope of "if (IsSupported(GLFeature::robustness))". I think they can be merged.

Comment 20

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8844704 [details]
Bug 1339256 - Double-check robustness. -

https://reviewboard.mozilla.org/r/118016/#review119848

> You might have a mistake on writing a redundant scope of "if (IsSupported(GLFeature::robustness))". I think they can be merged.

The first scope can MarkUnsupported the feature, causing the second scope not to run.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed97037dae9c
Detect robust_buffer_access_behavior. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/86fe1c44ac5a
Simplify index validation. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6bb817bae08
Double-check robustness. - r=daoshengmu

Comment 22

[Iris Hsiao [:ihsiao]]

backed out for build bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=82387475&repo=mozilla-inbound&lineNumber=5774

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bb0c61a3ee3

Comment 23

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112216/diff/2-3/

Comment 24

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/5-6/

Comment 25

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8844704 [details]
Bug 1339256 - Double-check robustness. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118016/diff/1-2/

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a168a559693
Detect robust_buffer_access_behavior. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/ddb0bfb55854
Simplify index validation. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/79d152f90a49
Double-check robustness. - r=daoshengmu

Comment 27

[Bob Clary [:bc] (inactive)]

This broke start up on Android devices for Autophone with crashes [@ mozilla::gl::GLContextEGLFactory::Create]

Comment 28

[Bob Clary [:bc] (inactive)]

This appears to have affected Android 4.4 and later but not Android 4.2 and the Android 4.3 emulators.

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=android&fromchange=1673d1fc6164d14517a61401968fe9188c4a73a1

Comment 29

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1b822c69468163ce8f7dc5c0da8aed529dbf3d3c for the autophone issues.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #29)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 1b822c69468163ce8f7dc5c0da8aed529dbf3d3c for the autophone issues.

Messed up the commit message for the backout, it really did back out all three commits.

Comment 31

[Bob Clary [:bc] (inactive)]

Although the S1S2 and Talos tests have issues reporting the crash properly, the Unit tests do give it:

https://treeherder.mozilla.org/logviewer.html#?job_id=82619012&repo=mozilla-inbound&lineNumber=434

Comment 32

[Bob Clary [:bc] (inactive)]

Confirmed green after the backout.

Comment 33

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1339256 - Only request robustness if requested on EGL -

Review commit: https://reviewboard.mozilla.org/r/118832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118832/

Comment 34

[Kelsey Gilbert [:jgilbert]]

Attachment: Bug 1339256 - Don't assert on unrecognized context reset strategy -

Review commit: https://reviewboard.mozilla.org/r/118834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/118834/

Comment 35

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112216/diff/3-4/

Comment 36

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/112218/diff/6-7/

Comment 37

[Kelsey Gilbert [:jgilbert]]

Comment on attachment 8844704 [details]
Bug 1339256 - Double-check robustness. -

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/118016/diff/2-3/

Comment 38

[Intermittent Failures Robot]

24 failures in 137 pushes (0.175 failures/push) were associated with this bug yesterday.  
Repository breakdown:
* mozilla-inbound: 24

Platform breakdown:
* android-6-0-armv8-api15: 14
* android-4-4-armv7-api15: 6
* android-7-1-armv8-api15: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339256&startday=2017-03-09&endday=2017-03-09&tree=all

Comment 39

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8845683 [details]
Bug 1339256 - Don't assert on unrecognized context reset strategy -

https://reviewboard.mozilla.org/r/118834/#review120786

Comment 40

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8845682 [details]
Bug 1339256 - Only request robustness if requested on EGL -

https://reviewboard.mozilla.org/r/118832/#review120790

LGTM

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/09487aeae4a7
Detect robust_buffer_access_behavior. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/539928251430
Simplify index validation. - r=kvark
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b8cdf0d9c1f
Double-check robustness. - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cb755b4421d
Only request robustness if requested on EGL - r=daoshengmu
https://hg.mozilla.org/integration/mozilla-inbound/rev/c52c06620add
Don't assert on unrecognized context reset strategy - r=daoshengmu

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/09487aeae4a7
https://hg.mozilla.org/mozilla-central/rev/539928251430
https://hg.mozilla.org/mozilla-central/rev/3b8cdf0d9c1f
https://hg.mozilla.org/mozilla-central/rev/6cb755b4421d
https://hg.mozilla.org/mozilla-central/rev/c52c06620add

Comment 43

[Intermittent Failures Robot]

24 failures in 790 pushes (0.03 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 24

Platform breakdown:
* android-6-0-armv8-api15: 14
* android-4-4-armv7-api15: 6
* android-7-1-armv8-api15: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1339256&startday=2017-03-06&endday=2017-03-12&tree=all