846848 - [OS.File] Handle UNC-formatted paths

Status: VERIFIED FIXED

(Toolkit Graveyard :: OS.File, defect)

Comment 1

[Bablu Kumar]

I want to work on this bug.I am well acquainted with the knowledge of c/c++.I think it this first bug would be a great opportunity for me.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Hi (again) Bablu Kumar.
That's great, too :) As mentioned, I will be mostly unreachable until March 26th, so I will not be able to follow this work closely.

If you wish to get started, you should get familiar with UNC-formatted paths. These are described here:
http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx

The code that needs to be modified is here:
http://dxr.mozilla.org/mozilla-central/toolkit/components/osfile/ospath_win_back.jsm

Note that this is JavaScript work.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths

Given time constraints, I will handle this bug myself.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 726700 [details] [diff] [review]
UNC-formatted paths

Review of attachment 726700 [details] [diff] [review]:
-----------------------------------------------------------------

Windows paths were one of those things I hoped I could avoid.

::: toolkit/components/osfile/ospath_win_back.jsm
@@ +82,5 @@
> +         // The path is reduced to a UNC drive
> +         if (noDrive) {
> +           return ".";
> +         } else {
> +           return path;

Updating the doc comment for this case would be good, since we're no longer returning everything before the last \\.

@@ +153,5 @@
> +         if (index == -1) {
> +           return path;
> +         }
> +         return path.slice(0, index);
> +       }

This handling of UNC paths appears to contradict the documentation comment: we're no longer restricted to returning single letters (if we ever were, since the path.slice call below looks like it can return longer strings).

Doc comment needs fixing in any event; this function needs more tests.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #4)
> Comment on attachment 726700 [details] [diff] [review]
> UNC-formatted paths
> 
> Review of attachment 726700 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Windows paths were one of those things I hoped I could avoid.

Yes, me too, but it seems that real life is forcing our hand.

> ::: toolkit/components/osfile/ospath_win_back.jsm
> @@ +82,5 @@
> > +         // The path is reduced to a UNC drive
> > +         if (noDrive) {
> > +           return ".";
> > +         } else {
> > +           return path;
> 
> Updating the doc comment for this case would be good, since we're no longer
> returning everything before the last \\.

Well, that's the case except in the corner case in which the path is "\\\\DriveName". I'll try and update the documentation accordingly, though.

> 
> @@ +153,5 @@
> > +         if (index == -1) {
> > +           return path;
> > +         }
> > +         return path.slice(0, index);
> > +       }
> 
> This handling of UNC paths appears to contradict the documentation comment:
> we're no longer restricted to returning single letters (if we ever were,
> since the path.slice call below looks like it can return longer strings).
> 
> Doc comment needs fixing in any event; this function needs more tests.

Added a few more tests. Do you have additional ideas?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v2

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 727196 [details] [diff] [review]
UNC-formatted paths, v2

Review of attachment 727196 [details] [diff] [review]:
-----------------------------------------------------------------

A bit of rs+, but OK.  We should have more tests for winGetDrive.

::: toolkit/components/osfile/ospath_win_back.jsm
@@ +61,5 @@
>        *
> +      * Otherwise, return everything before the last backslash,
> +      * including the drive/server name.
> +      *
> +      *

Nit: extra whitespace.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 727196 [details] [diff] [review]
UNC-formatted paths, v2

(forgot a file)

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v3

Fixes, more tests, got tests out of the main test suite.
Given that this changes the patch quite a lot, Nathan, if you have a few minutes, I would be glad if you could review this. Otherwise, I will keep your r+ from last time.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=0a805c2bfd29

Comment 11

[Nathan Froyd [:froydnj]]

Comment on attachment 728985 [details] [diff] [review]
UNC-formatted paths, v3

Review of attachment 728985 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me on moving the testing stuff around.  f=me for everything else; if I'm missing something on the questions below, we can upgrade that to an r=me.

::: toolkit/components/osfile/ospath_win_back.jsm
@@ +29,5 @@
> +    Components.utils.import("resource://gre/modules/osfile/osfile_win_allthreads.jsm", this);
> +  } catch (ex) {
> +    Components.utils.reportError(ex);
> +    // Executing ospath_win_back.jsm without a kernel.dll is acceptable for testing purposes
> +  }

This makes me a little nervous.  Can we check for a more specific error, maybe by catching the original error in *allthreads and throwing a more specific one?

@@ +55,5 @@
> +         if (index == 1) {
> +           return "";
> +         }
> +         path = path.slice(index);
> +         // We need at least another "\\" to have a basename

Where are you getting this "another \" from?  For:

\\server\path\to\my\folder

you exit the if with |path = "\\folder"|, correct?

I think the UNC case *does* do what you want in a roundabout way, but it's not clear to me what the comment is for.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #11)
> Comment on attachment 728985 [details] [diff] [review]
> UNC-formatted paths, v3
> 
> Review of attachment 728985 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> rs=me on moving the testing stuff around.  f=me for everything else; if I'm
> missing something on the questions below, we can upgrade that to an r=me.
> 
> ::: toolkit/components/osfile/ospath_win_back.jsm
> @@ +29,5 @@
> > +    Components.utils.import("resource://gre/modules/osfile/osfile_win_allthreads.jsm", this);
> > +  } catch (ex) {
> > +    Components.utils.reportError(ex);
> > +    // Executing ospath_win_back.jsm without a kernel.dll is acceptable for testing purposes
> > +  }
> 
> This makes me a little nervous.  Can we check for a more specific error,
> maybe by catching the original error in *allthreads and throwing a more
> specific one?

Good point. I will do that.

If we want absolute safety, I could do the following:
- in the xpcshell test, add some preference (e.g. "osfile.testing.paths = true");
- in the code, read preference osfile.testing.paths, and only catch the exception if the preference is set.

> 
> @@ +55,5 @@
> > +         if (index == 1) {
> > +           return "";
> > +         }
> > +         path = path.slice(index);
> > +         // We need at least another "\\" to have a basename
> 
> Where are you getting this "another \" from?  For:
> 
> \\server\path\to\my\folder
> 
> you exit the if with |path = "\\folder"|, correct?
> 
> I think the UNC case *does* do what you want in a roundabout way, but it's
> not clear to me what the comment is for.

I will try and simplify that code.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #12)
> (In reply to Nathan Froyd (:froydnj) from comment #11)
> > This makes me a little nervous.  Can we check for a more specific error,
> > maybe by catching the original error in *allthreads and throwing a more
> > specific one?
> 
> Good point. I will do that.
> 
> If we want absolute safety, I could do the following:
> - in the xpcshell test, add some preference (e.g. "osfile.testing.paths =
> true");
> - in the code, read preference osfile.testing.paths, and only catch the
> exception if the preference is set.

I think either one of these is reasonable; the pref might even make it a little more obvious what's going on.

> > @@ +55,5 @@
> > > +         if (index == 1) {
> > > +           return "";
> > > +         }
> > > +         path = path.slice(index);
> > > +         // We need at least another "\\" to have a basename
> > 
> > Where are you getting this "another \" from?  For:
> > 
> > \\server\path\to\my\folder
> > 
> > you exit the if with |path = "\\folder"|, correct?
> > 
> > I think the UNC case *does* do what you want in a roundabout way, but it's
> > not clear to me what the comment is for.
> 
> I will try and simplify that code.

I don't know that the code needs to be simplified that much; just:

  return path.slice(index+1);

in the UNC case might be sufficient.  But hm, would we want to say that the basename of:

\\server

is "server"?  That doesn't make much sense...or is just \\server on its own not a valid path?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v4

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v5

Taking the opportunity to slightly simplify the code a little further.

Comment 16

[Nathan Froyd [:froydnj]]

Comment on attachment 729146 [details] [diff] [review]
UNC-formatted paths, v5

Review of attachment 729146 [details] [diff] [review]:
-----------------------------------------------------------------

WFM, although I feel like I have looked at a non-qref'd version of the patch?

::: toolkit/components/osfile/osfile_unix_allthreads.jsm
@@ +51,5 @@
>      }
>    }
>    if (!libc) {
> +    // Note: If you change the string here, please adapt tests accordingly
> +    throw new Error("Could not open system library: no libc");

Since we're using a pref, this change isn't necessary, right?

::: toolkit/components/osfile/osfile_win_allthreads.jsm
@@ +43,5 @@
> +    libc = ctypes.open("kernel32.dll");
> +  } catch (ex) {
> +    // Note: If you change the string here, please adapt consumers and
> +    // tests accordingly
> +    throw new Error("Could not open system library: " + ex.message);

...and likewise unnecessary here because we're using a pref?

::: toolkit/components/osfile/ospath_unix_back.jsm
@@ +15,5 @@
>   * - all path concatenations go through function |join|.
>   */
>  if (typeof Components != "undefined") {
>    this.EXPORTED_SYMBOLS = ["OS"];
> +  Components.utils.import("resource://gre/modules/Services.jsm", this);

I wonder if it'd be a better idea to import this into a local variable, so that we're not holding a long-lived Services reference from OS.File.  Does that sound crazy?

@@ +29,5 @@
> +  }
> +
> +  try {
> +    Components.utils.import("resource://gre/modules/osfile/osfile_unix_allthreads.jsm", this);
> +  } catch (ex if syslib_not_necessary && ex.becauseNoSysLib) {

I assume becauseNoSysLib is a relic from an earlier idea?

::: toolkit/components/osfile/tests/xpcshell/test_path.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/Services.jsm", this);
> +Services.prefs.setBoolPref("osfile.test.syslib_not_necessary", true);

Nit: I think this should be toolkit.osfile.test.syslib_required with a value of false.  Update other places accordingly, please.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

(In reply to Nathan Froyd (:froydnj) from comment #16)
> Comment on attachment 729146 [details] [diff] [review]
> UNC-formatted paths, v5
> 
> Review of attachment 729146 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> WFM, although I feel like I have looked at a non-qref'd version of the patch?
> 
> ::: toolkit/components/osfile/osfile_unix_allthreads.jsm
> @@ +51,5 @@
> >      }
> >    }
> >    if (!libc) {
> > +    // Note: If you change the string here, please adapt tests accordingly
> > +    throw new Error("Could not open system library: no libc");
> 
> Since we're using a pref, this change isn't necessary, right?

Well, it makes sure that we are not trapping other errors. I can roll this back, but it makes me feel slightly less dirty.

> ::: toolkit/components/osfile/osfile_win_allthreads.jsm
> @@ +43,5 @@
> > +    libc = ctypes.open("kernel32.dll");
> > +  } catch (ex) {
> > +    // Note: If you change the string here, please adapt consumers and
> > +    // tests accordingly
> > +    throw new Error("Could not open system library: " + ex.message);
> 
> ...and likewise unnecessary here because we're using a pref?

As above.

> ::: toolkit/components/osfile/ospath_unix_back.jsm
> @@ +15,5 @@
> >   * - all path concatenations go through function |join|.
> >   */
> >  if (typeof Components != "undefined") {
> >    this.EXPORTED_SYMBOLS = ["OS"];
> > +  Components.utils.import("resource://gre/modules/Services.jsm", this);
> 
> I wonder if it'd be a better idea to import this into a local variable, so
> that we're not holding a long-lived Services reference from OS.File.  Does
> that sound crazy?

That sounds good.
 
> @@ +29,5 @@
> > +  }
> > +
> > +  try {
> > +    Components.utils.import("resource://gre/modules/osfile/osfile_unix_allthreads.jsm", this);
> > +  } catch (ex if syslib_not_necessary && ex.becauseNoSysLib) {
> 
> I assume becauseNoSysLib is a relic from an earlier idea?

Oops.

> ::: toolkit/components/osfile/tests/xpcshell/test_path.js
> @@ +4,5 @@
> > +
> > +"use strict";
> > +
> > +Components.utils.import("resource://gre/modules/Services.jsm", this);
> > +Services.prefs.setBoolPref("osfile.test.syslib_not_necessary", true);
> 
> Nit: I think this should be toolkit.osfile.test.syslib_required with a value
> of false.  Update other places accordingly, please.

You really don't like no double negations, don't you? :)

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v6

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=3af72e6fb977

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #17)
> (In reply to Nathan Froyd (:froydnj) from comment #16)
> > Since we're using a pref, this change isn't necessary, right?
> 
> Well, it makes sure that we are not trapping other errors. I can roll this
> back, but it makes me feel slightly less dirty.

Ah, yes.  I think I saw this part, balked, but then understood what was going on when I read later bits of the patch.  And failed to go back and update.  This change is OK.

> > ::: toolkit/components/osfile/osfile_win_allthreads.jsm
> > @@ +43,5 @@
> > > +    libc = ctypes.open("kernel32.dll");
> > > +  } catch (ex) {
> > > +    // Note: If you change the string here, please adapt consumers and
> > > +    // tests accordingly
> > > +    throw new Error("Could not open system library: " + ex.message);
> > 
> > ...and likewise unnecessary here because we're using a pref?
> 
> As above.

Ditto.

> > > +Components.utils.import("resource://gre/modules/Services.jsm", this);
> > > +Services.prefs.setBoolPref("osfile.test.syslib_not_necessary", true);
> > 
> > Nit: I think this should be toolkit.osfile.test.syslib_required with a value
> > of false.  Update other places accordingly, please.
> 
> You really don't like no double negations, don't you? :)

No I don't. :)

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 729812 [details] [diff] [review]
UNC-formatted paths, v6

Review of attachment 729812 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the changes below.  Don't forget to fix *all* the instances of the pref name. :)

::: toolkit/components/osfile/ospath_unix_back.jsm
@@ +19,5 @@
> +  let Scope = {};
> +  Components.utils.import("resource://gre/modules/Services.jsm", Scope);
> +
> +  // Some tests need to import this module from any platform.
> +  // We detect this by setting a bogus preference "osfile.test.syslib_not_necessary"

Fix the comment to reflect the correct pref name, please. :)

::: toolkit/components/osfile/ospath_win_back.jsm
@@ +30,5 @@
> +
> +  // Some tests need to import this module from any platform.
> +  // We detect this by setting a bogus preference "osfile.test.syslib_not_necessary"
> +  // from the test suite
> +  let syslib_necessary = false;

Need to change the comment and the default value here.

::: toolkit/components/osfile/tests/xpcshell/test_path.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/Services.jsm", this);
> +Services.prefs.setBoolPref("osfile.test.syslib_not_necessary", true);

Three points:

- The pref should end with .syslib_necessary;
- The pref should be set to false;
- The pref's full name should be toolkit.osfile.test.syslib_necessary.  The toolkit. prefix is already established as a top-level namespace in prefs, so let's use that instead of inventing our own top-level namespace, which might have dire consequences someday...

Comment 22

[David Teller [:Yoric] - still alive but not very active]

Attachment: UNC-formatted paths, v7

Try: https://tbpl.mozilla.org/?tree=Try&rev=78c03ac22916

Well, that was surprising long.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Sorry, wrong try link: https://tbpl.mozilla.org/?tree=Try&rev=3af72e6fb977

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8db756a2bcd1

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8db756a2bcd1

Comment 26

[:Gavin Sharp [email: [e-mail]]]

We'll want to uplift this to beta to address bug 857672.

Comment 27

[Gregory Szorc [:gps]]

I also just realized this bug may break FHR data submission as well. How exactly, I'd have to test. But, I don't think it would be good.

Comment 28

[Lukas Blakk [:lsblakk] use ?needinfo]

Tracking for FF21, see comment 26.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Attachment: Patch for uplift to mozilla-release

(same patch, fixing trivial bitunrot)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Possibly bug 828223.
User impact if declined: Major. Address bar will not work for Windows users whose profile resides in a network share.
Testing completed (on m-c, etc.): Patch has resided on m-c for 6 days without apparent breakage.
Risk to taking this patch (and alternatives if risky): Normally, none.
String or IDL/UUID changes made by this patch: None.


Try: https://tbpl.mozilla.org/?tree=Try&rev=d5e071fc95b9

Comment 30

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #29)
> [Approval Request Comment]
> Bug caused by (feature/regressing bug #): Possibly bug 828223.

To be clear, *this* bug was "caused by" the landing of OS.File. Bug 846839, bug 857588 and bug 857672 were caused by switching consumers to OS.File while it had this bug (bug 828223 is one of many bugs where we did that).

> User impact if declined: Major. Address bar will not work for Windows users
> whose profile resides in a network share.

See also other bugs in the dependency list.

Comment 31

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #30)
> > User impact if declined: Major. Address bar will not work for Windows users
> > whose profile resides in a network share.
> 
> See also other bugs in the dependency list.

I guess I should also note that given the exceptions in bug 857672 comment 18 and subsequent comments, there is likely substantially more that's broken with the browser than just the address bar (and other bugs filed so far). The search service and session store are completely broken for these users, for example, and there are likely other side effects that are perhaps less user-visible.

Comment 32

[Lukas Blakk [:lsblakk] use ?needinfo]

Asked in IRC as well - but who can do testing on this to confirm the landing verifiably fixes the issue(s)?  Is there someone who can either help QA get set up (very quickly) with a test env, or do the testing and report back?

Comment 33

[Gregory Szorc [:gps]]

It's my understanding that the following steps could be used to verify this bug:

1) Find a Windows machine
2) Create a directory anywhere on the machine
3) In Windows Explorer, right click on that directory and select "Share With" -> "specific people..."
4) Select the account you are currently using
5) Finish that dialog. Be sure to select read/write.

At this point, that folder is being exported to the network so only the current user can access it.

You should be able to access the exported folder via the path \\machine-name\\export-name. e.g. my local Windows dev box is named "bourbon" and my share was "test-profile". You can access that path via \\bourbon\\test-profile.

Now, you're all set to start testing. When you set up a new Firefox profile, use \\bourbon\\test-profile as the path to the profile. I /think/ that should "just work."

Please note that depending on what shell you are in, you may need to quote '\\' or escape the backslashes. I've never tried creating a profile on a share before. Good luck!

Comment 34

[u279076]

Verified fixed in Firefox Aurora 22.0a2 2013-04-04 by verifying that this build is unaffected by bug 857672. Please advise if this bug warrants more testing than that.

Comment 35

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Rajchenbach Teller [:Yoric] from comment #29)
> Risk to taking this patch (and alternatives if risky): Normally, none.

Since we're likely going to ship this patch in a 20.0.1 to be released next week, I wanted to elaborate on the reasoning behind this risk evaluation (which I concur with). This patch makes minimal non-fix-related changes (some minor changes to other platform back-ends for testing purposes), and the fix-related changes themselves are pretty isolated to handling UNC specifically (adding branches for path.charAt(0) == "\\"), which suggests a low likelihood of having negative impact in other scenarios. The automated test coverage for this component is also quite good.

Comment 36

[David Teller [:Yoric] - still alive but not very active]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #34)
> Verified fixed in Firefox Aurora 22.0a2 2013-04-04 by verifying that this
> build is unaffected by bug 857672. Please advise if this bug warrants more
> testing than that.

You may wish to test with http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-d5e071fc95b9 (that's Firefox 20.0 + the patch).

Comment 37

[brentpirolli]

Yoric, I downloaded those files for Win32, copied/pasted them over my previous FF 20.0 install's files, and it opened with the "nightly" graphics on it... and all works as intended. Bug has been resolved.  No errors in error console when launching new pages, and the version reports as Nightly 20.0.

Comment 38

[u279076]

(In reply to David Rajchenbach Teller [:Yoric] from comment #36)
> You may wish to test with
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dteller@mozilla.
> com-d5e071fc95b9 (that's Firefox 20.0 + the patch).

As with Brent, I confirm that this build resolves the issue reported in bug 857672. I think it's safe to land across all affected branches.

Comment 39

[Alex Keybl [:akeybl]]

Comment on attachment 733238 [details] [diff] [review]
Patch for uplift to mozilla-release

In preparation for the possibility of a 20.0.1.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8b5ca19d2df7

Note that this landed on m-c before the merge, so this is already on Aurora.

Comment 41

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get a testcase here for other platforms?  Would be great to ensure this works on more than just windows before shipping in a release.

Comment 42

[u279076]

(In reply to [e-mail] from comment #41)
> Can we get a testcase here for other platforms?  Would be great to ensure
> this works on more than just windows before shipping in a release.

I don't think so; neither Mac nor Linux use \\server\share naming conventions.

Linux connects to the same share using smb:///server/share/folder
Mac OSX automatically mounts the share at /Volumes/share

In neither case does Firefox 20 reproduce this bug. Unless someone can come up with a testcase which reproduces this bug on Linux and Mac using UNC paths I don't think we can verify this on anything else other than Windows.

Comment 43

[:Gavin Sharp [email: [e-mail]]]

UNC paths are a Windows-only thing.

Comment 44

[David Teller [:Yoric] - still alive but not very active]

I'd like to mark this checkin-needed.
Also, given that this fixes a Windows-only bug, I believe that any chemspill release could be Windows-only.

Comment 45

[:Gavin Sharp [email: [e-mail]]]

(undoing unintentional flag changes)

Comment 46

[Paul Silaghi, QA [:pauly]]

Verified fixed using the STR in https://bugzilla.mozilla.org/show_bug.cgi?id=857672#c50 on FF 21b2.

Comment 47

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 733238 [details] [diff] [review]
Patch for uplift to mozilla-release

Now that this has been verified on 21.0b2 - let's get uplift to mozilla-release branch, to the default branch.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/5b43de27b369

Comment 49

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi [QA] from comment #46)
> Verified fixed using the STR in
> https://bugzilla.mozilla.org/show_bug.cgi?id=857672#c50 on FF 21b2.
Verified fixed FF 20.0.1 Win XP, Win 7.

Comment 50

[Lukas Blakk [:lsblakk] use ?needinfo]

This bug has been listed as part of the 20.0.1 release notes in https://www.mozilla.org/en-US/firefox/20.0.1/releasenotes/. If you find any of the information or wording incorrect in the notes, please email [e-mail] asap. Thanks!