[CRD iOS] Occasional crash when connecting to Mac host. Potentially bug in the GL API |
|||||||||||
Issue descriptionWhat steps will reproduce the problem? (1) Connect to a Mac host (2) Enter trackpad mode (3) Move cursor to the bottom right corner of the screen There is a ~20% chance the app will crash. No idea why Mac host makes it easier to crash... Here is the stack trace: https://paste.googleplex.com/5462681603538944 In this specific crash: Desktop size: 1024, 768 Updated rect: left = 92, top = 740, right = 324, bottom = 768 Note that right and bottom are exclusive. Memory address of frame.data(): 0x1108f8000 GlRenderLayer::UpdateTexture() does this: ... glPixelStorei(GL_UNPACK_ROW_LENGTH, 1024); glTexSubImage2D(GL_TEXTURE_2D, 0, /*offset_x*/ 92, /*offset_y*/ 740, /*width*/ 232, /*height*/ 28, GL_RGBA, GL_UNSIGNED_BYTE, buffer_to_update); ... IIUC, the last byte glTexSubImage2D should access is 0x1108f8000 + ((92 + 740*1024) + ((232-1) + (28-1)*1024))*4 = 0x110BF750C. However, glTexSubImage2D attempted to access address 0x110bf8000, which is higher than the highest allocated memory address (0x110bf7ffc) and had caused EXC_BAD_ACCESS. This seems to be a bug in glTexSubImage2D. I'll file a bug in radar.
,
Oct 26 2017
I don't have stats but it seems to be very uncommon (<5%?) and TBH very hard to repro. `new uint8_t[]` usually returns a chunk of memory with an accessible range much larger than the requested size so for most of the time read access will not cause segfault even if it's out of bound... I tried to repro that by constantly updating a subregion near the bottom but so far have no luck to repro that issue...
,
Oct 26 2017
Okay... That percentage doesn't seem to be low... I got it after the fourth try... Desktop size: 1024, 768 offset_x = 240, offset_y = 760, width = 46, height = 8 Memory address of frame.data(): 0x11bcac000 Upper bound (exclusive) of frame.data(): 0x11bfac000 Expected last address to access: 0x11BFAB474 Attempted to access: 0x11bfac000 Could we somehow add padding to the uint8_t array when being created in BasicDesktopFrame? But TBH I don't know how much padding is needed before knowing how their glTexSubImage2D implementation works... It also could be that I misunderstood how glTexSubImage2D works... Here is the doc: https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/glTexSubImage2D.xhtml which says: The texels referenced by pixels replace the portion of the existing texture array with x indices xoffset and xoffset+width−1xoffset+width-1, inclusive, and y indices yoffset and yoffset+height−1yoffset+height-1, inclusive. This region may not include any texels outside the range of the texture array as it was originally specified. I think it's still Apple's bug in the implementation?
,
Oct 27 2017
Looks like when connecting to the host for Apple review (macOS VM, 1024x768 resolution) from iOS, the failure rate is about ~30%. On Android I could not repro that after 20 connection attempts. For now we probably need to work around this by implementing a PaddedDesktopFrame that adds extra space at the end of the buffer (I'll start with one extra row).
,
Oct 27 2017
,
Oct 27 2017
,
Oct 27 2017
,
Oct 27 2017
,
Oct 27 2017
Verified using mprotect that this bug only affects iOS but not Android, and it's likely that the function only attempts to read on extra row.
,
Oct 27 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9cbdf3dd95459f3f581c94fae599083b272f8aee commit 9cbdf3dd95459f3f581c94fae599083b272f8aee Author: Yuwei Huang <yuweih@chromium.org> Date: Fri Oct 27 23:36:09 2017 [CRD iOS] Workaround memory access bug in glTexSubImage2D The implementation of glTexSubImage2D from iOS seems to be buggy. It attempts to read memory that is out of the expected upper bound: kBytesPerPixel * (width + (height - 1) * stride). Given that SIGSEGV only happens when offset_y + subimage_height == desktop_height, it seems they attempt to read one more row of buffer for no obvious reason (no extra artifact compared to Android). This CL introduces a PaddedDesktopFrame to workaround this problem by padding the desktop frame data with one extra row, so that glTexSubImage2D doesn't land on protected memory. Bug: 778550 Change-Id: I250012e4fc6c43bb99c277a0275912c1ca120d51 Reviewed-on: https://chromium-review.googlesource.com/742622 Reviewed-by: Jamie Walch <jamiewalch@chromium.org> Commit-Queue: Yuwei Huang <yuweih@chromium.org> Cr-Commit-Position: refs/heads/master@{#512349} [modify] https://crrev.com/9cbdf3dd95459f3f581c94fae599083b272f8aee/remoting/client/dual_buffer_frame_consumer.cc
,
Oct 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/74c36d2dde28556e53dae226e52a370a07950b69 commit 74c36d2dde28556e53dae226e52a370a07950b69 Author: Findit <findit-for-me@appspot.gserviceaccount.com> Date: Sat Oct 28 00:10:01 2017 Revert "[CRD iOS] Workaround memory access bug in glTexSubImage2D" This reverts commit 9cbdf3dd95459f3f581c94fae599083b272f8aee. Reason for revert: Findit (https://goo.gl/kROfz5) identified CL at revision 512349 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyRAsSDVdmU3VzcGVjdGVkQ0wiMWNocm9taXVtLzljYmRmM2RkOTU0NTlmM2Y1ODFjOTRmYWU1OTkwODNiMjcyZjhhZWUM Sample Failed Build: https://ci.chromium.org/buildbot/chromium.mac/Mac%20Builder/77199 Original change's description: > [CRD iOS] Workaround memory access bug in glTexSubImage2D > > The implementation of glTexSubImage2D from iOS seems to be buggy. It > attempts to read memory that is out of the expected upper bound: > kBytesPerPixel * (width + (height - 1) * stride). Given that SIGSEGV > only happens when offset_y + subimage_height == desktop_height, it > seems they attempt to read one more row of buffer for no obvious > reason (no extra artifact compared to Android). > > This CL introduces a PaddedDesktopFrame to workaround this problem > by padding the desktop frame data with one extra row, so that > glTexSubImage2D doesn't land on protected memory. > > Bug: 778550 > Change-Id: I250012e4fc6c43bb99c277a0275912c1ca120d51 > Reviewed-on: https://chromium-review.googlesource.com/742622 > Reviewed-by: Jamie Walch <jamiewalch@chromium.org> > Commit-Queue: Yuwei Huang <yuweih@chromium.org> > Cr-Commit-Position: refs/heads/master@{#512349} Change-Id: Ida1ed9bbfa64814e320468c4dd1280962e988c72 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 778550 Reviewed-on: https://chromium-review.googlesource.com/742351 Cr-Commit-Position: refs/heads/master@{#512362} [modify] https://crrev.com/74c36d2dde28556e53dae226e52a370a07950b69/remoting/client/dual_buffer_frame_consumer.cc
,
Oct 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9b8694693d28be082ed9457ac64a12cd8d541ee0 commit 9b8694693d28be082ed9457ac64a12cd8d541ee0 Author: Yuwei Huang <yuweih@chromium.org> Date: Mon Oct 30 20:36:20 2017 Reland "[CRD iOS] Workaround memory access bug in glTexSubImage2D" This is a reland of 9cbdf3dd95459f3f581c94fae599083b272f8aee Original change's description: > [CRD iOS] Workaround memory access bug in glTexSubImage2D > > The implementation of glTexSubImage2D from iOS seems to be buggy. It > attempts to read memory that is out of the expected upper bound: > kBytesPerPixel * (width + (height - 1) * stride). Given that SIGSEGV > only happens when offset_y + subimage_height == desktop_height, it > seems they attempt to read one more row of buffer for no obvious > reason (no extra artifact compared to Android). > > This CL introduces a PaddedDesktopFrame to workaround this problem > by padding the desktop frame data with one extra row, so that > glTexSubImage2D doesn't land on protected memory. > > Bug: 778550 > Change-Id: I250012e4fc6c43bb99c277a0275912c1ca120d51 > Reviewed-on: https://chromium-review.googlesource.com/742622 > Reviewed-by: Jamie Walch <jamiewalch@chromium.org> > Commit-Queue: Yuwei Huang <yuweih@chromium.org> > Cr-Commit-Position: refs/heads/master@{#512349} Bug: 778550 Change-Id: I2f6ea5db226aea7b68f34decf90fa7b97ed7d75c Reviewed-on: https://chromium-review.googlesource.com/742655 Reviewed-by: Jamie Walch <jamiewalch@chromium.org> Commit-Queue: Yuwei Huang <yuweih@chromium.org> Cr-Commit-Position: refs/heads/master@{#512608} [modify] https://crrev.com/9b8694693d28be082ed9457ac64a12cd8d541ee0/remoting/client/BUILD.gn [modify] https://crrev.com/9b8694693d28be082ed9457ac64a12cd8d541ee0/remoting/client/dual_buffer_frame_consumer.cc
,
Oct 30 2017
,
Oct 30 2017
Rerunning app for 5 times, and connecting to the host 5 times per launch. Couldn't repro the problem any more. The workaround should be working now.
,
Oct 31 2017
The apple bug can be tracked at radar://35211059
,
Nov 9 2017
,
Dec 12 2017
,
Dec 12 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by jamiewa...@chromium.org
, Oct 26 2017