Bug 41510 - Add Base64DecodePolicy option at base64Decode()
Summary: Add Base64DecodePolicy option at base64Decode()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 42061 (view as bug list)
Depends on:
Blocks: 41462
  Show dependency treegraph
 
Reported: 2010-07-02 00:52 PDT by Patrick R. Gansterer
Modified: 2010-10-04 22:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.41 KB, patch)
2010-07-02 01:23 PDT, Patrick R. Gansterer
ap: review-
ap: commit-queue-
Details | Formatted Diff | Diff
Patch (12.45 KB, patch)
2010-07-03 14:05 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
testcase (37.78 KB, text/html)
2010-07-04 08:05 PDT, Patrick R. Gansterer
no flags Details
Patch (12.88 KB, patch)
2010-08-02 09:15 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (added null-string handling) (12.96 KB, patch)
2010-08-02 11:00 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (15.85 KB, patch)
2010-08-08 09:51 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (remove argument name "policy") (15.83 KB, patch)
2010-08-08 10:00 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (16.26 KB, patch)
2010-08-31 10:35 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (from linux) (16.28 KB, patch)
2010-09-01 14:40 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (15.89 KB, patch)
2010-09-08 17:59 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
crash log from bot (39.76 KB, text/plain)
2010-09-13 15:30 PDT, Eric Seidel (no email)
no flags Details
Patch (16.97 KB, patch)
2010-09-14 08:33 PDT, Patrick R. Gansterer
darin: commit-queue-
Details | Formatted Diff | Diff
Patch (17.05 KB, patch)
2010-09-14 09:27 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch for nil value (1.25 KB, patch)
2010-09-14 11:27 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch for nil value (4.51 KB, patch)
2010-09-14 11:37 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch for nil value (4.51 KB, patch)
2010-09-14 13:53 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff
Patch (14.58 KB, patch)
2010-09-16 12:08 PDT, Patrick R. Gansterer
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch (14.62 KB, patch)
2010-10-02 07:14 PDT, Patrick R. Gansterer
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Patrick R. Gansterer 2010-07-02 00:52:08 PDT
see patch
Comment 1 Patrick R. Gansterer 2010-07-02 01:23:47 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 2 Alexey Proskuryakov 2010-07-02 12:02:46 PDT
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.
Comment 3 Alexey Proskuryakov 2010-07-02 12:03:56 PDT
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 4 Darin Adler 2010-07-02 12:05:52 PDT
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
Comment 5 Darin Adler 2010-07-02 12:14:44 PDT
(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.
Comment 6 Darin Adler 2010-07-02 12:17:16 PDT
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”.
Comment 7 Patrick R. Gansterer 2010-07-03 14:05:49 PDT
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.
Comment 8 Patrick R. Gansterer 2010-07-03 14:27:33 PDT
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?
Comment 9 Darin Adler 2010-07-03 16:23:19 PDT
(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.
Comment 10 Patrick R. Gansterer 2010-07-04 08:05:26 PDT
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
Comment 11 Darin Adler 2010-07-04 09:20:09 PDT
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.
Comment 12 Patrick R. Gansterer 2010-07-04 09:21:55 PDT
(In reply to comment #11)
Which is the "correct" one I should use now?
Comment 13 Alexey Proskuryakov 2010-07-12 11:45:30 PDT
See also: bug 42061. Let's decide which one to keep.
Comment 14 Patrick R. Gansterer 2010-08-02 09:15:03 PDT
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 15 Darin Adler 2010-08-02 09:54:01 PDT
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?
Comment 16 Patrick R. Gansterer 2010-08-02 10:22:13 PDT
(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?
Comment 17 Darin Adler 2010-08-02 10:40:19 PDT
(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.
Comment 18 Patrick R. Gansterer 2010-08-02 10:44:53 PDT
(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?
Comment 19 Darin Adler 2010-08-02 10:47:22 PDT
(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.
Comment 20 Patrick R. Gansterer 2010-08-02 11:00:50 PDT
Created attachment 63238 [details]
Patch (added null-string handling)
Comment 21 Darin Adler 2010-08-02 11:02:47 PDT
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 22 WebKit Commit Bot 2010-08-04 09:44:13 PDT
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
Comment 23 Patrick R. Gansterer 2010-08-08 09:51:32 PDT
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.
Comment 24 Patrick R. Gansterer 2010-08-08 10:00:38 PDT
Created attachment 63845 [details]
Patch (remove argument name "policy")

(In reply to comment #21)
> Normally we would omit the argument name "policy" here.
Comment 25 Eric Seidel (no email) 2010-08-09 03:16:43 PDT
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.
Comment 26 Patrick R. Gansterer 2010-08-31 10:35:48 PDT
Created attachment 66070 [details]
Patch

rebased to head
Comment 27 Darin Adler 2010-08-31 10:38:13 PDT
Comment on attachment 66070 [details]
Patch

> +        * platform/text/Base64.h:
> +        (WebCore::):

Strange stray line here.
Comment 28 WebKit Commit Bot 2010-08-31 13:57:52 PDT
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
Comment 29 Patrick R. Gansterer 2010-09-01 14:40:25 PDT
Created attachment 66276 [details]
Patch (from linux)

regenerated patch on linux
maybe it will apply now
Comment 30 WebKit Commit Bot 2010-09-01 23:23:15 PDT
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
Comment 31 Kwang Yul Seo 2010-09-05 15:10:49 PDT
*** Bug 42061 has been marked as a duplicate of this bug. ***
Comment 32 Patrick R. Gansterer 2010-09-08 17:59:53 PDT
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 33 Adam Roben (:aroben) 2010-09-09 08:03:20 PDT
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 34 WebKit Commit Bot 2010-09-09 09:37:13 PDT
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 35 Eric Seidel (no email) 2010-09-10 23:04:49 PDT
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 36 Eric Seidel (no email) 2010-09-10 23:04:54 PDT
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.
Comment 37 Eric Seidel (no email) 2010-09-13 15:30:52 PDT
Created attachment 67480 [details]
crash log from bot
Comment 38 Patrick R. Gansterer 2010-09-14 08:33:09 PDT
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 39 Darin Adler 2010-09-14 09:09:21 PDT
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.
Comment 40 Patrick R. Gansterer 2010-09-14 09:13:48 PDT
(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?
Comment 41 Patrick R. Gansterer 2010-09-14 09:27:11 PDT
Created attachment 67566 [details]
Patch

Corrected encoding.
Comment 42 Darin Adler 2010-09-14 09:34:32 PDT
(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.
Comment 43 Patrick R. Gansterer 2010-09-14 09:58:27 PDT
(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.
Comment 44 Darin Adler 2010-09-14 10:55:00 PDT
(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.
Comment 45 Patrick R. Gansterer 2010-09-14 11:11:01 PDT
(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!
Comment 46 Darin Adler 2010-09-14 11:12:39 PDT
(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 = @"";
Comment 47 Patrick R. Gansterer 2010-09-14 11:27:42 PDT
Created attachment 67578 [details]
Patch for nil value

(In reply to comment #46)
> It would be:
> 
>     if (!locationString)
>         locationString = @"";
THX!
Comment 48 Patrick R. Gansterer 2010-09-14 11:37:29 PDT
Created attachment 67579 [details]
Patch for nil value

Ok, now with LayoutTest
Comment 49 Patrick R. Gansterer 2010-09-14 13:53:10 PDT
Created attachment 67599 [details]
Patch for nil value

Should apply now.
Comment 50 Patrick R. Gansterer 2010-09-16 09:27:55 PDT
@Darin: Can you r+ attachment 67599 [details]?
Comment 51 WebKit Commit Bot 2010-09-16 11:19:12 PDT
Comment on attachment 67599 [details]
Patch for nil value

Clearing flags on attachment: 67599

Committed r67642: <http://trac.webkit.org/changeset/67642>
Comment 52 Patrick R. Gansterer 2010-09-16 12:08:27 PDT
Created attachment 67826 [details]
Patch
Comment 53 WebKit Commit Bot 2010-09-17 14:20:55 PDT
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 54 Eric Seidel (no email) 2010-09-17 15:00:08 PDT
Comment on attachment 67826 [details]
Patch

Flaky test?
Comment 55 WebKit Commit Bot 2010-09-17 15:53:08 PDT
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 56 Eric Seidel (no email) 2010-09-18 03:19:56 PDT
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.
Comment 57 Patrick R. Gansterer 2010-10-02 07:14:45 PDT
Created attachment 69572 [details]
Patch

Added missing encodeURI() at the new tests.
Comment 58 WebKit Commit Bot 2010-10-04 22:06:10 PDT
Comment on attachment 69572 [details]
Patch

Clearing flags on attachment: 69572

Committed r69072: <http://trac.webkit.org/changeset/69072>
Comment 59 WebKit Commit Bot 2010-10-04 22:06:19 PDT
All reviewed patches have been landed.  Closing bug.