Closed Bug 1339256 Opened 3 years ago Closed 3 years ago

Explicitly detect robust_buffer_access_behavior

Categories

(Core :: Canvas: WebGL, defect, P1)

Product:
Core
Component:
Canvas: WebGL
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: jgilbert, Assigned: jgilbert)

References

Details

(Whiteboard: gfx-noted)

Attachments

(5 files)

Bug 1339256 - Detect robust_buffer_access_behavior. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1339256 - Simplify index validation. -
59 bytes, text/x-review-board-request
kvark
: review+
Details
Bug 1339256 - Double-check robustness. -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1339256 - Only request robustness if requested on EGL -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details
Bug 1339256 - Don't assert on unrecognized context reset strategy -
59 bytes, text/x-review-board-request
daoshengmu
: review+
Details 
No description provided.
Comment on attachment 8836905 [details]
Bug 1339256 - Detect robust_buffer_access_behavior. -

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

LGTM
Attachment #8836905 - Flags: review?(dmu) → review+
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 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 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.
Attachment #8836906 - Flags: review?(kvark) → review+
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 on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

Non-trivial changes, but can't seem to revert the r+ to r? in reviewboard.
Attachment #8836906 - Flags: review+ → review?(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?
Attachment #8836906 - Flags: review?(kvark) → review+
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 on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

More review.
Previous versions didn't support BufferSubData updates!
Attachment #8836906 - Flags: review+ → review?(kvark)
Comment on attachment 8836906 [details]
Bug 1339256 - Simplify index validation. -

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

LGTM
Attachment #8836906 - Flags: review?(kvark) → review+
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.
Attachment #8844704 - Flags: review?(dmu) → review+
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.
Pushed by jgilbert@mozilla.com:
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
Blocks: 1345386
Pushed by jgilbert@mozilla.com:
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
This broke start up on Android devices for Autophone with crashes [@ mozilla::gl::GLContextEGLFactory::Create]
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
(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.
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
Confirmed green after the backout.
Comment on attachment 8845683 [details]
Bug 1339256 - Don't assert on unrecognized context reset strategy -

https://reviewboard.mozilla.org/r/118834/#review120786
Attachment #8845683 - Flags: review?(dmu) → review+
Comment on attachment 8845682 [details]
Bug 1339256 - Only request robustness if requested on EGL -

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

LGTM
Attachment #8845682 - Flags: review?(dmu) → review+
Pushed by jgilbert@mozilla.com:
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
No longer blocks: 1347249
Depends on: 1347249
Flags: needinfo?(jgilbert)
Depends on: 1427088
Depends on: 1476327
Blocks: 1477817
Blocks: 1482289
You need to log in before you can comment on or make changes to this bug.