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

RESOLVED FIXED

Status

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: phil, Assigned: bugzilla-graveyard)

Tracking

Details

Attachments

(1 attachment)

2.39 KB, patch
bugzilla-graveyard
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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

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

Comment 2

12 years ago
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)?
(Reporter)

Comment 3

12 years ago
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.
(Reporter)

Comment 4

12 years ago
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?
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Component: Accessibility → OS Integration
QA Contact: accessibility → os.integration
Summary: Cmd-U sometimes opens blank window → Opening a url via the service (cmd-shift-u) sometimes opens blank window
(Assignee)

Comment 6

12 years ago
No, we do not support whitespace characters as part of shortcuts.
Component: OS Integration → Accessibility
Component: Accessibility → OS Integration
(Assignee)

Comment 7

11 years ago
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.
Assignee: nobody → cl-bugs-new
Hardware: Macintosh → All
(Assignee)

Comment 8

11 years ago
Created attachment 333670 [details] [diff] [review]
fix v1
Attachment #333670 - Flags: review?(murph)
(Assignee)

Updated

11 years ago
Attachment #333670 - Flags: review?(murph) → review?(trendyhendy2000)
(Assignee)

Comment 9

11 years ago
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.
Attachment #333670 - Flags: review?(trendyhendy2000) → review+
(Assignee)

Updated

11 years ago
Attachment #333670 - Flags: superreview?(mikepinkerton)
+  // 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 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
Attachment #333670 - Flags: superreview?(mikepinkerton) → superreview-

Comment 12

11 years ago
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.
(Assignee)

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
Comment on attachment 333670 [details] [diff] [review]
fix v1

Re-requesting sr from pink now that explanations and an audit have been presented.
Attachment #333670 - Flags: superreview- → superreview?(mikepinkerton)
Comment on attachment 333670 [details] [diff] [review]
fix v1

sr=pink
Attachment #333670 - Flags: superreview?(mikepinkerton) → superreview+
Landed on cvs trunk.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.