Last Comment Bug 672128 - copying entire URL shouldn't copy "wyciwyg://" URIs or username/password
: copying entire URL shouldn't copy "wyciwyg://" URIs or username/password
: regression
Product: Firefox
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
-- normal (vote)
: Firefox 8
Assigned To: :Gavin Sharp [email:]
: Marco Bonardo [::mak]
Depends on:
Blocks: 665580
  Show dependency treegraph
Reported: 2011-07-17 12:29 PDT by Alice0775 White
Modified: 2011-08-19 08:03 PDT (History)
6 users (show) in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

test html (880 bytes, text/html)
2011-07-17 15:44 PDT, Alice0775 White
no flags Details
patch (6.68 KB, patch)
2011-07-18 22:15 PDT, :Gavin Sharp [email:]
dao+bmo: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description User image Alice0775 White 2011-07-17 12:29:46 PDT
Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:8.0a1) Gecko/20110717 Firefox/8.0a1 ID:20110717030802

When I was testing Bug 672119, I found this problem.

When I copy selected text in the location bar and paste, wyciwyg://x/ is added extra.
I am not sure this is intentional or not.

*This also happens Latest Aurora7.0a2.
*This is not happens on Firefox6.0beta build2.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox with New Profile (without addons)
2. Go to URL and Reload once.
3. Click "Recherche" on the left to display search fields.
4. Type "40" below "entrer le numero de l'adresse recherchée" and "rue blanche" below "entrer le nom de la rue".
5. Click "Rechercher".
6. Select text in the Locationbar of the popup window.
7. Copy selected text in the Locationbar
8. Paste it elsewhere.

Actual Results: 
  Copied text is as follows.

Expected Results:
  Should not expose wyciwyg://x/.
  Copied text should be as follows.

Regression window:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1 ID:20110623030811
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:7.0a1) Gecko/20110623 Firefox/7.0a1 ID:20110623013442
Comment 1 User image Thomas Ahlblom 2011-07-17 12:40:18 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110717 Firefox/8.0a1

Comment 3 User image Alice0775 White 2011-07-17 15:44:03 PDT
Created attachment 546446 [details]
test html
Comment 4 User image :Gavin Sharp [email:] 2011-07-18 22:15:04 PDT
Created attachment 546710 [details] [diff] [review]

Thanks for the great bug report, Alice!
Comment 5 User image :Gavin Sharp [email:] 2011-07-18 22:38:28 PDT
Comment on attachment 546710 [details] [diff] [review]

>diff --git a/browser/base/content/test/browser_wyciwyg_urlbarCopying.js b/browser/base/content/test/browser_wyciwyg_urlbarCopying.js

>+      isnot(currentURL.slice(wyciwyg.length), wyciwyg, currentURL + " is a wyciwyg URI");

Oops, this is obviously wrong (started with an isnot(indexOf("wyciwyg"), -1)), will fix locally.
Comment 6 User image :Gavin Sharp [email:] 2011-07-18 22:42:10 PDT
Changed to:
ok(/^wyciwyg:\/\//i.test(currentURL), currentURL + " is a wyciwyg URI");
Comment 7 User image Dão Gottwald [:dao] 2011-07-19 05:39:22 PDT
Comment on attachment 546710 [details] [diff] [review]

>--- a/browser/base/content/urlbarBindings.xml
>+++ b/browser/base/content/urlbarBindings.xml
>@@ -508,16 +508,20 @@
>           var selectedVal = inputVal.substring(this.selectionStart, this.selectionEnd);
>           // If the selection doesn't start at the beginning or URL bar is
>           // modified, nothing else to do here.
>           if (this.getAttribute("pageproxystate") != "valid" || this.selectionStart > 0)
>             return selectedVal;
>           let uri = gBrowser.currentURI;
>+          // Only copy exposable URIs
>+          try {
>+            uri = Cc[";1"].getService(Ci.nsIURIFixup).createExposableURI(uri);
>+          } catch (ex) {}

Should selectedVal be returned in the exception case? I guess not, after glancing at docshell/base/nsIURIFixup.idl.
Alternatively, add a comment on why it's ok to use the original uri when createExposableURI throws?
Comment 8 User image :Gavin Sharp [email:] 2011-07-21 11:04:28 PDT
The IDL documentation looks wrong, it seems like it will only really throw for malformed wyciwyg URIs (I can't imagine this actually occurs in practice). I'm tempted to just remove the try/catch, but I guess I'll just leave it as-is to match the behavior of XULBrowserWindow.onLocationChange.
Comment 9 User image :Gavin Sharp [email:] 2011-07-21 11:20:03 PDT
Comment 10 User image :Gavin Sharp [email:] 2011-07-21 11:49:29 PDT
Comment on attachment 546710 [details] [diff] [review]

I think we should get this fix on Aurora (primarily for the password copying variant). It only affects copying from the location bar, and it makes us match the behavior we have when setting the URL bar value, so the risk is quite low.
Comment 11 User image christian 2011-07-21 14:50:33 PDT
Comment on attachment 546710 [details] [diff] [review]

approved for releases/mozilla-aurora
Comment 12 User image :Gavin Sharp [email:] 2011-07-21 18:08:58 PDT
Comment 13 User image :Gavin Sharp [email:] 2011-07-21 18:15:45 PDT
Comment 14 User image Virgil Dicu [:virgil] [QA] 2011-08-19 08:03:35 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110818 Firefox/9.0a1

Tested with the steps provided in the description, as well as with the test html. The issue could not be reproduced anymore.

Setting status to verified fixed.

Note You need to log in before you can comment on or make changes to this bug.