Bug 209007 - SharedMemory::Handle::m_size should be more consistent
Summary: SharedMemory::Handle::m_size should be more consistent
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-03-12 11:47 PDT by David Kilzer (:ddkilzer)
Modified: 2020-03-17 20:32 PDT (History)
3 users (show)

See Also:


Attachments
Patch v1 (2.59 KB, patch)
2020-03-12 12:55 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v2 (2.59 KB, patch)
2020-03-17 13:54 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch v3 (2.57 KB, patch)
2020-03-17 15:34 PDT, David Kilzer (:ddkilzer)
darin: review+
ddkilzer: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2020-03-12 11:47:51 PDT
SharedMemory::Handle::m_size should be more consistent.

<rdar://problem/60340890>
Comment 1 David Kilzer (:ddkilzer) 2020-03-12 12:55:29 PDT
Created attachment 393401 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2020-03-12 12:56:26 PDT
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 3 David Kilzer (:ddkilzer) 2020-03-12 13:17:40 PDT
Comment on attachment 393401 [details]
Patch v1

Marking cq- until bots finish chewing on this.
Comment 4 David Kilzer (:ddkilzer) 2020-03-13 13:55:39 PDT
Comment on attachment 393401 [details]
Patch v1

Need figure out why tests are failing.
Comment 5 David Kilzer (:ddkilzer) 2020-03-16 16:27:46 PDT
(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.
Comment 6 David Kilzer (:ddkilzer) 2020-03-17 13:54:08 PDT
Created attachment 393782 [details]
Patch v2

Same as Patch v1 with stderr logging added to run on EWS bots.
Comment 7 David Kilzer (:ddkilzer) 2020-03-17 15:25:55 PDT
(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.
Comment 8 David Kilzer (:ddkilzer) 2020-03-17 15:34:33 PDT
Created attachment 393796 [details]
Patch v3
Comment 9 Darin Adler 2020-03-17 15:44:55 PDT
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 10 David Kilzer (:ddkilzer) 2020-03-17 20:23:11 PDT
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.
Comment 11 David Kilzer (:ddkilzer) 2020-03-17 20:32:51 PDT
Committed r258620: <https://trac.webkit.org/changeset/258620>