| Summary: | Add Base64DecodePolicy option at base64Decode() | ||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Patrick R. Gansterer <paroga> | ||||||||||||||||||||||||||||||||||||||
| Component: | WebCore Misc. | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | ap, commit-queue, darin, joybro201, skyul | ||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||||||||||||||||||||||
| Bug Blocks: | 41462 | ||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||
|
Description
Patrick R. Gansterer
2010-07-02 00:52:08 PDT
Created attachment 60348 [details]
Patch
The change in DOMWindow is covered by atob-btoa.html
What is the best way to do a LayoutTest for the change in Page.cpp? Example?
Comment on attachment 60348 [details] Patch > What is the best way to do a LayoutTest for the change in Page.cpp? Example? It would help if ChangeLog explained the change. There is a number of calls for user style sheet manipulation in layoutTestController, such as addUserStyleSheet, clearPersistentUserStyleSheet, setPersistentUserStyleSheetLocation, setUserStyleSheetEnabled, setUserStyleSheetLocation. + if (!base64Decode(in, out, false)) { The person reading this code will have no idea about what "false" means. Please use a named enum constant instead. r- for lack of testing and ChangeLog explanation. I didn't check the actual math. See also: loosely related bug 23566. Even more importantly, we'll need to know why this change is made. Is it for compatibility with other browsers? If so, then which? Comment on attachment 60348 [details] Patch Looks pretty good. > - if (!base64Decode(in, out)) { > + if (!base64Decode(in, out, false)) { When a boolean argument is one that we'll pass constants to, we normally prefer an enum. There's no way to see what that "false" means at the call site. So we'd do this: enum WhitespacePolicy { FailIfWhitespaceIsEncountered, SkipWhitespace }; Then make the argument type be WhitespacePolicy. > - // Note: Keep this in sync with the "out_len" computation below. > + // Note: Keep this in sync with the "outLen" computation below. As long as you have to rename this, I think I would have avoided the abbreviation and named it outLength. > + // Allowed because String::ascii() converts invalid charcters to '?', Typo: "charcters". > + // which isn't contained in the base64 characterset. Typo: "characterset". I suggest this comment: // The ascii() function turns any non-ASCII characters into "?" characters, // which yields correct behavior because those are illegal in base 64. > // 4-byte to 3-byte conversion > - unsigned outLen = len - ((len + 3) / 4); > + unsigned outLen = len - whitespaceCount - ((len + 3) / 4); I believe this is wrong. It should be "len - whitespaceCount - ((len - whitespaceCount + 3) / 4)". In cases with a lot of whitespace, the code you wrote would result in a buffer overrun! Maybe you could find a way to write it that would make that clear. Perhaps add a buffer-overrun-related assertion, and make sure you have a test case with a ton of whitespace in it. review- because of the buffer overrun issue As far as testing the use of a data URL for a user stylesheet, I don't think we have a good way to regression-test that at the moment -- we'd have to add something (In reply to comment #2) > There is a number of calls for user style sheet manipulation in layoutTestController, such as addUserStyleSheet, clearPersistentUserStyleSheet, setPersistentUserStyleSheetLocation, setUserStyleSheetEnabled, setUserStyleSheetLocation. Good point! I think using these we can test the data URL support in user style sheets, so happily it seems likely I was wrong about testing. Something critical to check on is that our definition of whitespace matches the specification and other browsers exactly. Also, Alexey pointed me to bug 23566 and some specification text there states that in at least some circumstances all unrecognized characters should be allowed and ignored. I am not sure we need a special case for whitespace in particular at all. Perhaps the feature we need is “skip/tolerate non-base-64 characters”. Created attachment 60456 [details] Patch > I am not sure we need a special case for whitespace in particular at all. Perhaps the feature we need is “skip/tolerate non-base-64 characters”. Other browsers ignore only whitespace. RFC 2396: > In some cases, extra whitespace (spaces, linebreaks, tabs, etc.) may > need to be added to break long URI across lines. The whitespace > should be ignored when extracting the URI. There is no exact definition what whitespace is. Is there a reason why user-stylesheet-fast-path.html lives in platform/mac/fast/loader/? If not I'll create a patch to move it into the platform independed folder? (In reply to comment #7) > There is no exact definition what whitespace is. OK. Then lets check the other browsers to see exactly what kinds of whitespace they skip. Created attachment 60472 [details] testcase (In reply to comment #9) > OK. Then lets check the other browsers to see exactly what kinds of whitespace they skip. IE 8: %09, %0A, %0D, %20 FF 3.6: %08, %09, %0A, %0D, %20 Safari 5.0: %09, %0A, %0B, %0C, %0D, %20 Chrome 6.0: %09, %0A, %0D, %20 Opera 10.53 & QtWebKit 4.6: all non-base64 (%00-%2A, %2C-%2E, %3A-%3C, %3E-%40, %5B-%60, %7B-%FF) CAUTION: IE supports only 31 stylesheet per page A quick aside, putting this in terms of our existing whitespace functions: It looks like Opera and Qt are matching the specification and tolerating anything unrecognized. Safari is matching isASCIISpace (which matches POSIX isspace). IE and Chrome have a whitespace definition more like the isWhitespace function in SVGParserUtilities.h, CSSPreloadScanner.cpp, LegacyPreloadScanner.cpp, and XPathFunction.cpp. Firefox’s concept of data URL whitespace doesn’t match any function already existing in WebKit. None of the browsers match isCSSWhitespace from CSSParser.cpp, isWhitespace from CompositeEditCommand.cpp, isTokenizerWhitespace from HTMLTokenizer.cpp, or isTreeBuilderWhiteSpace from HTMLTreeBuilder.cpp. (In reply to comment #11) Which is the "correct" one I should use now? See also: bug 42061. Let's decide which one to keep. Created attachment 63224 [details]
Patch
FIXED: With Base64DecodePolicy::FailOnInvalidCharacter a call to base64Decode(const String&,...) didn't work, because String::ascii() is null-terminated. Now remove the null-character at the end.
Comment on attachment 63224 [details] Patch > + // The ascii() function turns any non-ASCII characters into "?" characters, > + // which yields correct behavior because those are illegal in base 64. The ascii() function also turns U+0001-U+001F and U+007F into "?" characters. And turns the null string into "(null impl)". Are those behaviors desirable here? Do we have test cases that cover these? (In reply to comment #15) > (From update of attachment 63224 [details]) > > + // The ascii() function turns any non-ASCII characters into "?" characters, > > + // which yields correct behavior because those are illegal in base 64. > > The ascii() function also turns U+0001-U+001F and U+007F into "?" characters. And turns the null string into "(null impl)". Are those behaviors desirable here? Do we have test cases that cover these? In U+0001-U+001F is not valid base64 charachter; so no problem. What is the expected behaviour for null-stings? I think like a empty string? Or return false for base64Decode? (In reply to comment #16) > What is the expected behaviour for null-stings? I think like a empty string? Or return false for base64Decode? My general preference is to handle null strings identically to empty strings except in specific cases where we want different handling for them. For this specific function, I think we should double check what the call sites need. (In reply to comment #17) > (In reply to comment #16) > > What is the expected behaviour for null-stings? I think like a empty string? Or return false for base64Decode? > > My general preference is to handle null strings identically to empty strings except in specific cases where we want different handling for them. > > For this specific function, I think we should double check what the call sites need. What should I do with the code now? Is implementing a handling for null-characters enough, to get this in? (In reply to comment #18) > What should I do with the code now? Is implementing a handling for null-characters enough, to get this in? You added the use of ascii() so you’re introducing this new behavior. You need to check that it’s OK. That’s all. So did you check? Is it OK? I’ll review the rest of the patch soon. Created attachment 63238 [details]
Patch (added null-string handling)
Comment on attachment 63238 [details] Patch (added null-string handling) > +bool base64Decode(const String&, Vector<char>&, Base64DecodePolicy policy = FailOnInvalidCharacter); > +bool base64Decode(const Vector<char>&, Vector<char>&, Base64DecodePolicy policy = FailOnInvalidCharacter); > +bool base64Decode(const char*, unsigned, Vector<char>&, Base64DecodePolicy policy = FailOnInvalidCharacter); Normally we would omit the argument name "policy" here. Comment on attachment 63238 [details] Patch (added null-string handling) Rejecting patch 63238 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--ignore-tests', 'compositing', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20776 test cases. fast/dom/Window/atob-btoa.html -> failed Exiting early after 1 failures. 6794 tests run. 134.39s total testing time 6793 test cases (99%) succeeded 1 test case (<1%) had incorrect layout Full output: http://queues.webkit.org/results/3621499 Created attachment 63844 [details]
Patch
I found an additional error (in the existing code) and fixed it.
Also avoid the memcpy of the string now.
Created attachment 63845 [details] Patch (remove argument name "policy") (In reply to comment #21) > Normally we would omit the argument name "policy" here. Comment on attachment 63238 [details] Patch (added null-string handling) Cleared Darin Adler's review+ from obsolete attachment 63238 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 66070 [details]
Patch
rebased to head
Comment on attachment 66070 [details] Patch > + * platform/text/Base64.h: > + (WebCore::): Strange stray line here. Comment on attachment 66070 [details] Patch Rejecting patch 66070 from commit-queue. Failed to run "[u'/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1 Last 500 characters of output: ast/dom/Window/atob-btoa-expected.txt.rej (Stripping trailing CRs from patch.) patching file LayoutTests/fast/dom/Window/atob-btoa.html Hunk #1 FAILED at 48. 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/fast/dom/Window/atob-btoa.html.rej (Stripping trailing CRs from patch.) patching file LayoutTests/platform/mac/fast/loader/user-stylesheet-fast-path-expected.txt (Stripping trailing CRs from patch.) patching file LayoutTests/platform/mac/fast/loader/user-stylesheet-fast-path.html Full output: http://queues.webkit.org/results/3834209 Created attachment 66276 [details]
Patch (from linux)
regenerated patch on linux
maybe it will apply now
Comment on attachment 66276 [details] Patch (from linux) Rejecting patch 66276 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20898 test cases. platform/mac/fast/loader/user-stylesheet-fast-path.html -> crashed Exiting early after 1 failures. 17378 tests run. 650.68s total testing time 17377 test cases (99%) succeeded 1 test case (<1%) crashed 26 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3916035 *** Bug 42061 has been marked as a duplicate of this bug. *** Created attachment 66975 [details] Patch (In reply to comment #30) > Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 > Running build-dumprendertree > Compiling Java tests > make: Nothing to be done for `default'. > Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests > Testing 20898 test cases. > platform/mac/fast/loader/user-stylesheet-fast-path.html -> crashed Sorry for the delay. I tried the patch on three different linux systems (Qt and Gtk port), but I wasn't able to reproduce a crash. The only thing I found was a missing newline at the expected file. If patch will fail again I need more information than the commit queue provides :-/. Comment on attachment 66975 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=66975&action=prettypatch Patrick tells me this patch is the same as the last one except for some changed line endings and an extra line in the expected results. Comment on attachment 66975 [details] Patch Rejecting patch 66975 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--ignore-tests', 'compositing,media', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests Testing 20951 test cases. platform/mac/fast/loader/user-stylesheet-fast-path.html -> crashed Exiting early after 1 failures. 17408 tests run. 763.40s total testing time 17407 test cases (99%) succeeded 1 test case (<1%) crashed 26 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/3926330 Comment on attachment 66070 [details] Patch Cleared Darin Adler's review+ from obsolete attachment 66070 [details] so that this bug does not appear in http://webkit.org/pending-commit. Comment on attachment 66276 [details] Patch (from linux) Cleared Darin Adler's review+ from obsolete attachment 66276 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 67480 [details]
crash log from bot
Created attachment 67555 [details]
Patch
The only change since the last patch is the following (+ChangeLog):
--- a/WebKit/mac/WebView/WebPreferences.mm
+++ b/WebKit/mac/WebView/WebPreferences.mm
@@ -642,7 +642,8 @@ (-[WebPreferences setUserStyleSheetLocation:])
locationString = [URL _web_originalDataAsString];
}
- [self _setStringValue:locationString forKey: WebKitUserStyleSheetLocationPreferenceKey];
+ if (locationString)
+ [self _setStringValue:locationString forKey: WebKitUserStyleSheetLocationPreferenceKey];
}
- (BOOL)shouldPrintBackgrounds
This should avoid the crash:
*** Terminating app due to uncaught exception 'NSInvalidArgumentException', reason: '*** -[NSCFDictionary setObject:forKey:]: attempt to insert nil value (key: WebKitUserStyleSheetLocationPreferenceKey)'
Comment on attachment 67555 [details]
Patch
The EWS tried to apply the patch and failed, so it can’t be committed as-is.
The idea of not setting the user style sheet if the locationString is nil is a little strange. Maybe setting it to the empty string or removing the preference setting entirely would be appropriate.
(In reply to comment #39) > (From update of attachment 67555 [details]) > The EWS tried to apply the patch and failed, so it can’t be committed as-is. I'll rebase the patch > The idea of not setting the user style sheet if the locationString is nil is a little strange. Maybe setting it to the empty string or removing the preference setting entirely would be appropriate. I have no knowledge of ObjC :-/. Can you post me a snippet for the patch? Created attachment 67566 [details]
Patch
Corrected encoding.
(In reply to comment #40) > > The idea of not setting the user style sheet if the locationString is nil is a little strange. Maybe setting it to the empty string or removing the preference setting entirely would be appropriate. > I have no knowledge of ObjC :-/. Can you post me a snippet for the patch? I know lots about Objective-C, but I’m not sure this truly is an Objective-C question. Here are the questions we need to answer to get the code right: Under what circumstances can locationString end up nil? What is the best behavior for the API in that case? Is this a change from the behavior of the existing code? Once we answer those I can advise on how to write the code. It may be that your early exit is the right thing to do. By the way, in WebKit we normally use a coding style called early return so a version that uses return rather than nesting the _setStringValue call inside an if would be our preferred style. (In reply to comment #42) > Under what circumstances can locationString end up nil? What is the best behavior for the API in that case? Is this a change from the behavior of the existing code? I don't think that we change behaviour, because it crashes at the moment (not so cool behaviour ;-)) IMHO we should set it to an empty string, because this is the best (string) representation for invalid. If we can remove the setting that would be better (but I don't know how to do it). > By the way, in WebKit we normally use a coding style called early return. I normally try to use early return where possible. I saw such an if statement in the file and copied it. (In reply to comment #43) > (In reply to comment #42) > > Under what circumstances can locationString end up nil? What is the best behavior for the API in that case? Is this a change from the behavior of the existing code? > I don't think that we change behaviour, because it crashes at the moment (not so cool behaviour ;-)) The crash is not introduced by your changes? If this is a pre-existing bug, this fix should go in separately. Maybe you think “existing code” means an earlier version of your patch? I think maybe you misunderstood my question. What was the behavior of this method if called in this way before any of your changes? > IMHO we should set it to an empty string, because this is the best (string) representation for invalid. If we can remove the setting that would be better (but I don't know how to do it). Empty string sounds OK. > I saw such an if statement in the file and copied it. Understood. Not the best thing to do in this case, I think. (In reply to comment #44) > The crash is not introduced by your changes? I don't thinks so, but I don't have a mac to evaluate it. It was already hard enough to find a (possible?) fix. But maybe someone will send me one? ;-) > If this is a pre-existing bug, this fix should go in separately. I don't know what it fixes exactly, because I can check. :-( IMHO it should crash if you only apply my LayoutTest changes. I added test cases for invalid stylesheets, what maybe causes the crash. > Maybe you think “existing code” means an earlier version of your patch? existing code = SVN trunk > I think maybe you misunderstood my question. What was the behavior of this method if called in this way before any of your changes? I think a crash, but can't try. > > IMHO we should set it to an empty string, because this is the best (string) representation for invalid. If we can remove the setting that would be better (but I don't know how to do it). > > Empty string sounds OK. Can you post me a snippet for it? I'm not sure if a >>locationString = ""<< will work, because it is a NSURL*. > > I saw such an if statement in the file and copied it. > > Understood. Not the best thing to do in this case, I think. You're right, sorry! (In reply to comment #45) > > Empty string sounds OK. > Can you post me a snippet for it? I'm not sure if a >>locationString = ""<< will work, because it is a NSURL*. It’s an NSString, not an NSURL. It would be: if (!locationString) locationString = @""; Created attachment 67578 [details] Patch for nil value (In reply to comment #46) > It would be: > > if (!locationString) > locationString = @""; THX! Created attachment 67579 [details]
Patch for nil value
Ok, now with LayoutTest
Created attachment 67599 [details]
Patch for nil value
Should apply now.
@Darin: Can you r+ attachment 67599 [details]?
Comment on attachment 67599 [details] Patch for nil value Clearing flags on attachment: 67599 Committed r67642: <http://trac.webkit.org/changeset/67642> Created attachment 67826 [details]
Patch
Comment on attachment 67826 [details] Patch Rejecting patch 67826 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Projects/CommitQueue/LayoutTests Testing 21312 test cases. platform/mac/fast/loader/user-stylesheet-fast-path.html -> failed Exiting early after 1 failures. 17762 tests run. 334.59s total testing time 17761 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 31 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4044046 Comment on attachment 67826 [details]
Patch
Flaky test?
Comment on attachment 67826 [details] Patch Rejecting patch 67826 from commit-queue. Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--exit-after-n-failures=1', '--wait-for-httpd', '--quiet']" exit_code: 1 Running build-dumprendertree Compiling Java tests make: Nothing to be done for `default'. Running tests from /Projects/CommitQueue/LayoutTests Testing 21312 test cases. platform/mac/fast/loader/user-stylesheet-fast-path.html -> failed Exiting early after 1 failures. 17762 tests run. 321.84s total testing time 17761 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 31 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/4047043 Comment on attachment 66975 [details] Patch Cleared Adam Roben's review+ from obsolete attachment 66975 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 69572 [details]
Patch
Added missing encodeURI() at the new tests.
Comment on attachment 69572 [details] Patch Clearing flags on attachment: 69572 Committed r69072: <http://trac.webkit.org/changeset/69072> All reviewed patches have been landed. Closing bug. |