David Teller [:Yoric] - still alive but not very active Assignee
	Description • 11 years ago

      No description provided.

	Bablu Kumar
	Comment 1 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 2 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 3 • 11 years ago

Given time constraints, I will handle this bug myself.

	Nathan Froyd [:froydnj]
	Comment 4 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 5 • 11 years ago

(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?

	Nathan Froyd [:froydnj]
	Comment 7 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 8 • 11 years ago

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

(forgot a file)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 9 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 10 • 11 years ago

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

	Nathan Froyd [:froydnj]
	Comment 11 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 12 • 11 years ago

(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.

	Nathan Froyd [:froydnj]
	Comment 13 • 11 years ago

(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?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 15 • 11 years ago

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

	Nathan Froyd [:froydnj]
	Comment 16 • 11 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 17 • 11 years ago

(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? :)

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 11 years ago

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

	Nathan Froyd [:froydnj]
	Comment 20 • 11 years ago

(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. :)

	Nathan Froyd [:froydnj]
	Comment 21 • 11 years ago

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...

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 22 • 11 years ago

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

Well, that was surprising long.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 23 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 24 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 11 years ago

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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 26 • 11 years ago

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

	Gregory Szorc [:gps]
	Comment 27 • 11 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 28 • 11 years ago

Tracking for FF21, see comment 26.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 29 • 11 years ago

(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

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 30 • 11 years ago

(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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 31 • 11 years ago

(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 32 • 11 years ago

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?

	Gregory Szorc [:gps]
	Comment 33 • 11 years ago

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!

	u279076
	Comment 34 • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 35 • 11 years ago

(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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 36 • 11 years ago

(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/dteller@mozilla.com-d5e071fc95b9 (that's Firefox 20.0 + the patch).

	brentpirolli
	Comment 37 • 11 years ago

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.

	u279076
	Comment 38 • 11 years ago

(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.

	Alex Keybl [:akeybl]
	Comment 39 • 11 years ago

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

In preparation for the possibility of a 20.0.1.

	Ryan VanderMeulen [:RyanVM]
	Comment 40 • 11 years ago

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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 41 • 11 years ago

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.

	u279076
	Comment 42 • 11 years ago

(In reply to lsblakk@mozilla.com 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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 43 • 11 years ago

UNC paths are a Windows-only thing.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 44 • 11 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 45 • 11 years ago

(undoing unintentional flag changes)

	Paul Silaghi, QA [:pauly]
	Comment 46 • 11 years ago

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

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 47 • 11 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 48 • 11 years ago

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

	Paul Silaghi, QA [:pauly]
	Comment 49 • 11 years ago

(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.

	Lukas Blakk [:lsblakk] use ?needinfo
	Comment 50 • 11 years ago

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 release-mgmt@mozilla.com asap. Thanks!