391087 - Opening a url via the service (cmd-shift-u) sometimes opens blank window

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Phil Budne]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en; rv:1.8.1.4) Gecko/20070509 Camino/1.5

Cmd-U sometimes opens a window with a blank URL
(when camino already running)

Reproducible: Sometimes

Steps to Reproduce:
Camino running, no windows open?
highlight a URL
type Cmd-U
Actual Results:  
Camino window appears with blank URL, content


Expected Results:  
Open a new window with desired URL, content


Difficult to reproduce on demand

Started happening after an upgrade (to Camino 1.5, or an Apple O/S update)

Comment 1

[Derek Schrock]

You mean cmd-shift-u? The Camino service "Open URL in Camino."

Comment 2

[Stuart Morgan]

You'll need to elaborate on "sometimes": Always with certain URLs, but not with
others? Always once it starts happening until you quit Camino? Totally randomly
(i.e., if you select the same URL and do it over and over again, it will happen
some number of times)?

Comment 3

[Phil Budne]

Yes, I meant cmd-shift-U

The next time the problem occurs, I will try and gather statistics on how often I can cause it to repeat.

Comment 4

[Phil Budne]

I was just able to reproduce the problem 100% of the time after selecting the
ENTIRE next line (by triple clicking on it in a Terminal window):
    http://lists.ultimate.com/mailman/admindb/cccbb

Could the problem be the leading spaces?
I was able to open when I selected JUST the URL, but selecting
even one leading space I see the problem.

Camino ignores the end of line (which Safari does not-- it puts %0D%0A in the URL)
why not ignore leading spaces?

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

I can repro this with the steps in comment 4, so confirming; I'm pretty sure we should be doing *something* differently here.

I can't think of a valid reason not to strip leading spaces; we don't support a bookmark shortcut of " ", do we?

Comment 6

[Chris Lawson (gone)]

No, we do not support whitespace characters as part of shortcuts.

Comment 7

[Chris Lawson (gone)]

There's a bug in the resolveBookmarksShortcut method. A URL with leading spaces is (incorrectly) being resolved as a bookmarks shortcut, so the code path responsible for whitespace trimming in the openURL:userData:error: method never gets hit. Even if that code does get hit, though, it won't work, due to the same root cause as bug 411165. (NSScanner ignores whitespace by default, so the implementation of our NSString helper method was faulty.)

Patch coming for both of the bugs at work here. I may not be able to disentangle it from the patch for bug 411165, though.

Comment 8

[Chris Lawson (gone)]

Attachment: fix v1

Comment 9

[Chris Lawson (gone)]

Comment on attachment 333670 [details] [diff] [review]
fix v1

r=hendy on IRC. He needs Bugzilla privs, apparently, to be able to mark it r+, so I'm doing it for him.

Comment 10

[Mike Pinkerton (not reading bugmail)]

+  // Make sure we don't skip whitespace, which NSScanner does by default
+  [cleanerScanner setCharactersToBeSkipped:[NSCharacterSet characterSetWithCharactersInString:@""]];

this is a pretty sweeping change to a function we use all over the place. Are you sure there aren't any side-effects elsewhere where we might be relying on NSScanner's default behavior?

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 333670 [details] [diff] [review]
fix v1

minusing so it gets back on my radar later when you want me to take another look

Comment 12

[Stuart Morgan]

We certainly don't want the default behavior here; we have no idea what sets may be passed in, and my reading of the NSScanner docs is that when you try to do scanning for characters that are in the skip set the behavior is undefined. So if anything does change, it's because the caller is doing the wrong thing, (and I seriously doubt it would have been intentional to rely on undefined behavior).

In some cases, we are getting unintended behavior from the current code. Pasting into the web search field is supposed to strip control characters, but it also unexpectedly eats whitespace at the beginning. A bunch of code calls it with [NSCharacterSet whitespaceAndNewlineCharacterSet], and who knows what's happening there right now.

I think we should audit all the clients and figure out what we really want them to be doing (e.g., newlines pasted into the search field should probably become spaces), file any issues in follow-up bugs, then land this.

Comment 13

[Chris Lawson (gone)]

Audit, as requested per comment 12. I define "proper" or "correct" behaviour to be that which is desired (i.e., NOT skipping whitespace), and "current behaviour" to be what we're doing now (i.e., skipping whitespace):


1) WebFeatures.mm uses this in |addWhitelistSite:| as follows:

  NSString* host = [[mAddField stringValue] stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

We definitely want whitespace and newlines removed from a host name.


2) History.mm uses this in its formatter for the days field. A non-issue, as its sole purpose it to ensure only digits can be entered in that field.


3) NSString+Utils.m uses this in |stringByRemovingAmpEscapes|, which is used in importing Firefox bookmarks, among other things:

  return [dirtyStringMutant stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];

Yet another case where we definitely want to be removing these things entirely, so I think we're good here.


4) I introduce three instances in BookmarkItem.mm (see bug 450533), all as part of formatters ensuring input to the various bookmark editing interfaces is valid. All three rely on the correct behaviour described above and all three will probably fail with our current behaviour.


5) CertificateItem.mm uses this in |publicKeySizeBits|, as follows:

NSString* theKey = [[self publicKey] stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

I can't imagine this would have a proper behaviour if whitespace somehow ended up in there and was then skipped. Stuart, you know this code a lot better than I do. This needs the fix to work correctly, right?


6) We use it three times in BrowserSecurityDialogs.mm, all when evaluating the strength of passwords and not caring about whitespace. These uses should be indifferent to this change.


7) WebSearchField.mm's formatter currently looks like this:

- (BOOL)isPartialStringValid:(NSString *)partialString newEditingString:(NSString **)newString errorDescription:(NSString **)error
{
  if ([partialString rangeOfCharacterFromSet:[NSCharacterSet controlCharacterSet]].location != NSNotFound) {
    *newString = [partialString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];
    return NO;
  }
  return YES;
}

I agree with Stuart that we probably want to turn them into spaces rather than removing them entirely, but that just shifts the problem onto the other method (see bug 411165 comment 3). That makes this usage a non-issue, since we should change this to use |stringByReplacingCharactersInSet:withString:| rather than |stringByRemoving…|.


8) I introduce another usage of this in BrowserWindowController's new |cleanPastedText| method (again, see bug 411165). This usage relies on correct behaviour and will break if we keep the current behaviour.


9) AutoCompleteTextField's |performDragOperation:| relies on correct behaviour, not our present buggy behaviour, to strip control characters out of dragged text:

    dragString = [dragString stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];

Since that's going into the location bar and dealing with dragged NSString data that probably isn't a URL, we should instead change this to replace, rather than remove, so we don't end up with "some\rtext" turning into "sometext" (again, see bug 411165).


10) MainController's |openURL:userData:error:| method uses this to strip whitespace and newlines before it opens a URL that is not a shortcut:

    urlString = [urlString stringByRemovingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];
    [self showURL:urlString];

We definitely need proper behaviour here; it's just dumb luck that we haven't had a bug filed about this yet (although this bug itself may be that bug).


11) Finally, the |cleanedStringWithPastedString:| method I added to NSString+Utils relies on proper behaviour as well.


Point (7) definitely needs a followup bug, which I've filed as bug 452677. I'm not sure whether (9) needs a followup bug or not, as I've not determined how to test it yet.

Comment 14

[Chris Lawson (gone)]

Comment on attachment 333670 [details] [diff] [review]
fix v1

Re-requesting sr from pink now that explanations and an audit have been presented.

Comment 15

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 333670 [details] [diff] [review]
fix v1

sr=pink

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk.