Created attachment 393401 [details] Patch v1

Comment on attachment 393401 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=393401&action=review > Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:197 > + RELEASE_ASSERT(round_page(handle.m_size) == handle.m_size); This may not be strictly necessary after changes to WebKit::SharedMemory::Handle::decode(). Wanted a second opinion before removing it.

Comment on attachment 393401 [details] Patch v1 Marking cq- until bots finish chewing on this.

Comment on attachment 393401 [details] Patch v1 Need figure out why tests are failing.

(In reply to David Kilzer (:ddkilzer) from comment #4) > Comment on attachment 393401 [details] > Patch v1 > > Need figure out why tests are failing. The run-webkit-tests script says there's a crash, but results don't show a crash log: <https://ews-build.webkit.org/results/iOS-13-Simulator-WK2-Tests-EWS/r393401-13056-rerun/results.html> So it's likely the process may be returning early from SharedMemory::Handle::decode(). I haven't been able to reproduce this locally, but I was running macOS tests. I need to run the iOS Simulator tests next.

Created attachment 393782 [details] Patch v2 Same as Patch v1 with stderr logging added to run on EWS bots.

(In reply to David Kilzer (:ddkilzer) from comment #6) > Created attachment 393782 [details] > Patch v2 > > Same as Patch v1 with stderr logging added to run on EWS bots. Okay, in all of the failing tests I checked, this was printed: >>>>> SharedMemory::Handle::decode: size = 0, round_page(size) = 0 <https://ews-build.webkit.org/results/macOS-Mojave-Release-WK2-Tests-EWS/r393782-5352/results.html> So it seems that "0" is a valid size, so I just need to remove the `!size` check.

Created attachment 393796 [details] Patch v3

Comment on attachment 393796 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=393796&action=review > Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:197 > + RELEASE_ASSERT(round_page(handle.m_size) == handle.m_size); Not sure I understand why we need this to be RELEASE_ASSERT.

Comment on attachment 393796 [details] Patch v3 View in context: https://bugs.webkit.org/attachment.cgi?id=393796&action=review >> Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp:197 >> + RELEASE_ASSERT(round_page(handle.m_size) == handle.m_size); > > Not sure I understand why we need this to be RELEASE_ASSERT. The concern was that if someone (for example) changed SharedMemory::createHandle() or created a new way to build a SharedMemory::Handle that didn't follow this constraint, then the RELEASE_ASSERT would catch it. However, since no such code path exists, I'll change this back to a debug ASSERT() to document the invariant.

Committed r258620: <https://trac.webkit.org/changeset/258620>