Persona is no longer an option for authentication on BMO. For more details see Persona Deprecated.
Bug 846848 - [OS.File] Handle UNC-formatted paths
 Summary: [OS.File] Handle UNC-formatted paths
 Status: VERIFIED FIXED [fhr] Toolkit Components OS.File (show other bugs) Trunk All All -- normal (vote) mozilla22 David Teller [:Yoric] (please use "needinfo") Anthony Hughes (:ashughes) [GFX][QA][Mentor] David Teller [:Yoric] (please use "needinfo") (view as bug list) 846839 857588 857672 858634 Show dependency tree / graph

Reported: 2013-03-01 11:13 PST by David Teller [:Yoric] (please use "needinfo")
Modified: 2013-04-23 10:32 PDT (History)
17 users (show)
ryanvm: in‑testsuite+
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
 tracking-firefox20: + status-firefox20: verified tracking-firefox21: + status-firefox21: verified status-firefox22: verified relnote-firefox: 20+

Attachments
UNC-formatted paths (9.50 KB, patch)
2013-03-19 09:01 PDT, David Teller [:Yoric] (please use "needinfo")
nfroyd: review-
Details | Diff | Splinter Review
UNC-formatted paths, v2 (11.32 KB, patch)
2013-03-20 07:36 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
UNC-formatted paths, v3 (22.96 KB, patch)
2013-03-25 08:09 PDT, David Teller [:Yoric] (please use "needinfo")
nfroyd: feedback+
Details | Diff | Splinter Review
UNC-formatted paths, v4 (25.73 KB, patch)
2013-03-25 12:36 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
UNC-formatted paths, v5 (25.67 KB, patch)
2013-03-25 12:44 PDT, David Teller [:Yoric] (please use "needinfo")
nfroyd: feedback+
Details | Diff | Splinter Review
UNC-formatted paths, v6 (25.72 KB, patch)
2013-03-26 14:59 PDT, David Teller [:Yoric] (please use "needinfo")
nfroyd: review+
Details | Diff | Splinter Review
UNC-formatted paths, v7 (25.75 KB, patch)
2013-03-27 07:20 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Patch for uplift to mozilla-release (25.74 KB, patch)
2013-04-04 03:17 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
lukasblakk+bugs: approval‑mozilla‑release+
Details | Diff | Splinter Review

 David Teller [:Yoric] (please use "needinfo") 2013-03-01 11:13:09 PST Bablu Kumar 2013-03-14 13:34:14 PDT 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] (please use "needinfo") 2013-03-14 14:52:53 PDT 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] (please use "needinfo") 2013-03-19 09:01:36 PDT Created attachment 726700 [details] [diff] [review] UNC-formatted paths Given time constraints, I will handle this bug myself. Nathan Froyd [:froydnj] 2013-03-20 01:55:30 PDT 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] (please use "needinfo") 2013-03-20 07:36:09 PDT (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? David Teller [:Yoric] (please use "needinfo") 2013-03-20 07:36:35 PDT Created attachment 727196 [details] [diff] [review] UNC-formatted paths, v2 Nathan Froyd [:froydnj] 2013-03-21 09:23:15 PDT 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] (please use "needinfo") 2013-03-22 09:36:17 PDT Comment on attachment 727196 [details] [diff] [review] UNC-formatted paths, v2 (forgot a file) David Teller [:Yoric] (please use "needinfo") 2013-03-25 08:09:37 PDT Created attachment 728985 [details] [diff] [review] 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. David Teller [:Yoric] (please use "needinfo") 2013-03-25 08:11:36 PDT Try: https://tbpl.mozilla.org/?tree=Try&rev=0a805c2bfd29 Nathan Froyd [:froydnj] 2013-03-25 08:34:12 PDT 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] (please use "needinfo") 2013-03-25 09:32:51 PDT (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] 2013-03-25 09:39:04 PDT (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] (please use "needinfo") 2013-03-25 12:36:42 PDT Created attachment 729144 [details] [diff] [review] UNC-formatted paths, v4 David Teller [:Yoric] (please use "needinfo") 2013-03-25 12:44:34 PDT Created attachment 729146 [details] [diff] [review] UNC-formatted paths, v5 Taking the opportunity to slightly simplify the code a little further. Nathan Froyd [:froydnj] 2013-03-26 09:59:53 PDT 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] (please use "needinfo") 2013-03-26 14:57:10 PDT (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] (please use "needinfo") 2013-03-26 14:59:47 PDT Created attachment 729812 [details] [diff] [review] UNC-formatted paths, v6 David Teller [:Yoric] (please use "needinfo") 2013-03-26 15:00:41 PDT Try: https://tbpl.mozilla.org/?tree=Try&rev=3af72e6fb977 Nathan Froyd [:froydnj] 2013-03-26 18:06:27 PDT (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] 2013-03-27 06:37:00 PDT 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] (please use "needinfo") 2013-03-27 07:20:01 PDT Created attachment 730147 [details] [diff] [review] UNC-formatted paths, v7 Try: https://tbpl.mozilla.org/?tree=Try&rev=78c03ac22916 Well, that was surprising long. David Teller [:Yoric] (please use "needinfo") 2013-03-29 03:20:03 PDT Sorry, wrong try link: https://tbpl.mozilla.org/?tree=Try&rev=3af72e6fb977 Ryan VanderMeulen [:RyanVM] 2013-03-29 08:32:13 PDT https://hg.mozilla.org/integration/mozilla-inbound/rev/8db756a2bcd1 Ryan VanderMeulen [:RyanVM] 2013-03-30 17:56:52 PDT https://hg.mozilla.org/mozilla-central/rev/8db756a2bcd1 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-03 14:00:19 PDT We'll want to uplift this to beta to address bug 857672. Gregory Szorc [:gps] 2013-04-03 14:05:31 PDT 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 2013-04-03 19:35:51 PDT Tracking for FF21, see comment 26. David Teller [:Yoric] (please use "needinfo") 2013-04-04 03:17:33 PDT Created attachment 733238 [details] [diff] [review] 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 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-04 10:53:32 PDT (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] 2013-04-04 12:38:30 PDT (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 2013-04-04 13:01:24 PDT 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] 2013-04-04 13:10:41 PDT 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! Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-04-04 13:22:08 PDT 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] 2013-04-04 14:57:04 PDT (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] (please use "needinfo") 2013-04-04 23:48:48 PDT (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 2013-04-05 07:21:33 PDT 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. Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-04-05 10:08:27 PDT (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] 2013-04-05 13:01:47 PDT 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] 2013-04-05 13:55:25 PDT 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 2013-04-08 15:25:13 PDT 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. Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-04-08 16:14:57 PDT (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] 2013-04-08 20:07:37 PDT UNC paths are a Windows-only thing. David Teller [:Yoric] (please use "needinfo") 2013-04-08 21:22:20 PDT 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] 2013-04-08 22:24:26 PDT (undoing unintentional flag changes) Paul Silaghi, QA [:pauly] 2013-04-09 04:39:14 PDT Verified fixed using the STR in https://bugzilla.mozilla.org/show_bug.cgi?id=857672#c50 on FF 21b2. Lukas Blakk [:lsblakk] use ?needinfo 2013-04-09 11:56:38 PDT 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] 2013-04-09 12:07:55 PDT https://hg.mozilla.org/releases/mozilla-release/rev/5b43de27b369 Paul Silaghi, QA [:pauly] 2013-04-10 04:45:45 PDT (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 2013-04-11 11:41:46 PDT 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! Gregory Szorc [:gps] 2013-04-11 17:35:55 PDT *** Bug 846839 has been marked as a duplicate of this bug. ***

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