Closed
Bug 1339256
Opened 4 years ago
Closed 4 years ago
Explicitly detect robust_buffer_access_behavior
Categories
(Core :: Canvas: WebGL, defect, P1)
Core
Canvas: WebGL
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: jgilbert, Assigned: jgilbert)
References
Details
(Whiteboard: gfx-noted)
Attachments
(5 files)
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
kvark
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
daoshengmu
:
review+
|
Details |
No description provided.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 3•4 years ago
|
||
mozreview-review |
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 4•4 years ago
|
||
mozreview-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?
Assignee | ||
Comment 5•4 years ago
|
||
mozreview-review-reply |
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•4 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Assignee | ||
Comment 8•4 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 10•4 years ago
|
||
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 11•4 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 12•4 years ago
|
||
mozreview-review-reply |
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 hidden (mozreview-request) |
Assignee | ||
Comment 14•4 years ago
|
||
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 15•4 years ago
|
||
mozreview-review |
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•4 years ago
|
||
mozreview-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+
Assignee | ||
Comment 20•4 years ago
|
||
mozreview-review-reply |
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•4 years ago
|
||
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
Comment 22•4 years ago
|
||
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 26•4 years ago
|
||
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
Comment 27•4 years ago
|
||
This broke start up on Android devices for Autophone with crashes [@ mozilla::gl::GLContextEGLFactory::Create]
Comment 28•4 years ago
|
||
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
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/1b822c69468163ce8f7dc5c0da8aed529dbf3d3c for the autophone issues.
Flags: needinfo?(jgilbert)
(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•4 years ago
|
||
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•4 years ago
|
||
Confirmed green after the backout.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (Intermittent Failures Robot) |
Comment 39•4 years ago
|
||
mozreview-review |
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 40•4 years ago
|
||
mozreview-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+
Comment 41•4 years ago
|
||
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
Comment 42•4 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 4 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment hidden (Intermittent Failures Robot) |
Blocks: 1347249
Updated•4 years ago
|
Assignee | ||
Updated•4 years ago
|
Flags: needinfo?(jgilbert)
You need to log in
before you can comment on or make changes to this bug.
Description
•