Created attachment 338803[details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 338811[details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 338812[details]
Archive of layout-test-results from ews121 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 338820[details]
Archive of layout-test-results from ews114 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 338840[details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 339223[details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 339225[details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 339226[details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Attachment 339897[details] did not pass style-queue:
ERROR: Source/WebCore/platform/network/HTTPParsers.h:102: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4]
Total errors found: 1 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 339922[details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 339923[details]
Archive of layout-test-results from ews206 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 339995[details]
Archive of layout-test-results from ews102 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 339998[details]
Archive of layout-test-results from ews106 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 340000[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Created attachment 340007[details]
Archive of layout-test-results from ews117 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 340022[details]
Archive of layout-test-results from ews103 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 340031[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 340035[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.4
Comment on attachment 340042[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review
r=me if EWS is happy
> Source/WebCore/dom/Document.cpp:7815
> +void Document::setCrossOriginOptions(CrossOriginOptions value)
> +{
> + m_crossOriginOptions = value;
> + if (auto* window = domWindow())
> + window->setCrossOriginOptions(value);
> +}
Do we need to propagate m_crossOriginOptions inside Document::createDOMWindow() (covering the case where domWindow() is null at this point)?
Comment on attachment 340042[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review>> Source/WebCore/dom/Document.cpp:7815
>> +}
>
> Do we need to propagate m_crossOriginOptions inside Document::createDOMWindow() (covering the case where domWindow() is null at this point)?
No.
> Source/WebCore/page/DOMWindow.cpp:405
> + : AbstractDOMWindow(GlobalWindowIdentifier { Process::identifier(), generateObjectIdentifier<WindowIdentifierType>() }, document.crossOriginOptions())
because we already take care of grabbing the options from the Document when constructing the Window.
> Source/WebCore/page/DOMWindow.cpp:416
> + setCrossOriginOptions(document.crossOriginOptions());
And even when moving a DOMWindow from one document to another.
Comment on attachment 340042[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340042&action=review> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:229
> HRESULT setMenuItemElementEnabled([in] BOOL enabled);
}
[uuid(<<Generate and fill in some kind of UUID here>>)]
interface IWebPreferencesPrivate7 : IWebPreferencesPrivate6
{
> Source/WebKitLegacy/win/Interfaces/IWebPreferencesPrivate.idl:231
> + HRESULT setCrossOriginOptionsSupportEnabled([in] BOOL enabled);
You can't add these here without breaking existing WebKit clients on Windows (e.g., iTunes).
Instead, we have to create a new IWebPreferencesPrivate7 with these two new values.
> Source/WebKitLegacy/win/WebView.cpp:5152
>
COMPtr<IWebPreferencesPrivate7> prefsPrivate { Query, preferences };
> Source/WebKitLegacy/win/WebView.cpp:5158
> COMPtr<IWebPreferencesPrivate6> prefsPrivate { Query, preferences };
Then delete this one line ^^^
Comment on attachment 340042[details]
Patch
r- because this will break Windows clients. I've tried to give you the code changes to make for this to work safely.
(In reply to Chris Dumez from comment #57)
> Created attachment 340049[details]
> Patch
Turned on the feature by default as suggested by Geoff offline.
Windows port crashes soon. I'm going to fix it.
Bug 185505 – REGRESSION(r231622) [Win] Crashes for null dereference of prefsPrivate in WebView::notifyPreferencesChanged
Comment on attachment 340049[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340049&action=review> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:248
> + auto isCrossOriginAccess = [&] {
Not at a computer with a checkout at the moment. I take it the return value and error message can change after calling BindingSecurity::shouldAllowAccessToDOMWindow() more than once with the same arguments and hence we need to use a lambda and 2 local variable as opposed to just a 2 local variable to cache its result and error message?
> Source/WebCore/dom/Document.cpp:7812
> + m_crossOriginOptions = value;
Is it necessary to store this state on the document given that we also store it on the window?
> Source/WebCore/loader/FrameLoader.cpp:748
> + if (!crossOriginOptionsHeader.isEmpty())
This code said that and empty header is treated analogous to “allow”. For your consideration, I suggest that this logic be part of parseCrossOriginOptionsHeader() so as to centralize all the parsing and default semantics related to this header similar to how we parse X-Frame-Options and then update this code to only call the parse routine if crossOriginOptionsHeader is non-null (as the null string is an artifact of how ResourceResponse::httpHeaderField() works).
> Source/WebCore/platform/network/HTTPParsers.cpp:918
> + auto strippedHeader = stripLeadingAndTrailingHTTPSpaces(header);
I would have stored the return value in header as I do not feel that “stripped” adds much value, header is copied by value, and we can re-use the stack-space of the parameter instead of stack-allocating space for a local.
> Source/WebCore/platform/network/HTTPParsers.cpp:919
> + if (strippedHeader.isEmpty())
Look at this. You handle the empty string case. This conditional always evaluates to false because we never invoke this function when header is the empty string by line 748 of FrameLoader.cpp. And FrameLoader is the only caller of this function. See my remark in that file. If you have your heart set on keeping line 748 as-is then we should remove this line and line 920 and add an assertion that header is a non-empty string.
Even better, just remove this code and let us fall through to the default case on line 928.
> LayoutTests/http/wpt/cross-origin-options/cross-origin-options-header-expected.txt:3
> +PASS Cross-origin iframe with 'Cross-Origin-Options: deny' HTTP header
We should add tests for mIXED caSe header values since you use equalLettersIgnoringASCIICasein the parsing code. Also, we should add tests for when multiple Cross-Origin-Options are given in the same HTTP response.
Comment on attachment 340049[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=340049&action=review>> Source/WebCore/bindings/js/JSDOMWindowCustom.cpp:248
>> + auto isCrossOriginAccess = [&] {
>
> Not at a computer with a checkout at the moment. I take it the return value and error message can change after calling BindingSecurity::shouldAllowAccessToDOMWindow() more than once with the same arguments and hence we need to use a lambda and 2 local variable as opposed to just a 2 local variable to cache its result and error message?
Not sure I understand your comment. The result can change when called several times, which is why I cannot cache the result in a static.
The reason I have a lambda is to lazily initialize the cache value because there is one (common) code path that does not require a cross-origin check (There is a frame with this index and crossOriginOptions is Allow). Using a lambda avoids duplicating this code in other code paths.
>> Source/WebCore/dom/Document.cpp:7812
>> + m_crossOriginOptions = value;
>
> Is it necessary to store this state on the document given that we also store it on the window?
It'd be nice indeed but it is a little tricky because you can have a Document without a Window (and vice-versa). I'll dig deeper into it to see if this is feasible and follow-up if I can.
>> Source/WebCore/loader/FrameLoader.cpp:748
>> + if (!crossOriginOptionsHeader.isEmpty())
>
> This code said that and empty header is treated analogous to “allow”. For your consideration, I suggest that this logic be part of parseCrossOriginOptionsHeader() so as to centralize all the parsing and default semantics related to this header similar to how we parse X-Frame-Options and then update this code to only call the parse routine if crossOriginOptionsHeader is non-null (as the null string is an artifact of how ResourceResponse::httpHeaderField() works).
parseCrossOriginOptionsHeader() already does the right thing when passed an empty string so it would be fine to use isNull() here. I would do an unnecessary function call but this is not hot code anyway.
>> LayoutTests/http/wpt/cross-origin-options/cross-origin-options-header-expected.txt:3
>> +PASS Cross-origin iframe with 'Cross-Origin-Options: deny' HTTP header
>
> We should add tests for mIXED caSe header values since you use equalLettersIgnoringASCIICasein the parsing code. Also, we should add tests for when multiple Cross-Origin-Options are given in the same HTTP response.
Good idea, will extend testing.
2018-04-25 13:48 PDT, Chris Dumez
2018-04-25 14:57 PDT, EWS Watchlist
2018-04-25 15:43 PDT, EWS Watchlist
2018-04-25 15:50 PDT, EWS Watchlist
2018-04-25 16:29 PDT, EWS Watchlist
2018-04-25 16:51 PDT, Chris Dumez
2018-04-25 18:06 PDT, EWS Watchlist
2018-05-01 12:49 PDT, Chris Dumez
2018-05-01 14:04 PDT, EWS Watchlist
2018-05-01 14:40 PDT, EWS Watchlist
2018-05-01 15:02 PDT, EWS Watchlist
2018-05-01 16:36 PDT, Chris Dumez
2018-05-08 16:43 PDT, Chris Dumez
2018-05-08 19:45 PDT, EWS Watchlist
2018-05-08 20:01 PDT, EWS Watchlist
2018-05-09 09:54 PDT, Chris Dumez
2018-05-09 10:15 PDT, Chris Dumez
2018-05-09 10:28 PDT, Chris Dumez
2018-05-09 11:41 PDT, EWS Watchlist
2018-05-09 12:13 PDT, EWS Watchlist
2018-05-09 12:18 PDT, EWS Watchlist
2018-05-09 12:46 PDT, EWS Watchlist
2018-05-09 12:53 PDT, Chris Dumez
2018-05-09 14:10 PDT, EWS Watchlist
2018-05-09 14:41 PDT, EWS Watchlist
2018-05-09 14:50 PDT, EWS Watchlist
2018-05-09 15:10 PDT, Chris Dumez
2018-05-09 15:21 PDT, Chris Dumez
2018-05-09 15:31 PDT, Chris Dumez
2018-05-09 16:09 PDT, Chris Dumez
2018-05-09 16:12 PDT, Chris Dumez
2018-05-09 16:33 PDT, Chris Dumez