593815 - Auto-renaming at download is not working

Status: RESOLVED FIXED

(Core Graveyard :: File Handling, defect)

Description

[Ioana Chiorean]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; rv:2.0b5) Gecko/20100101 Firefox/4.0b5
Build Identifier: Build Identifier: Mozilla /5.0 (Android;Linux armv7l; rv:2.0b6pre)Gecko/20100906 Firefox/4.0b6pre Fennec /2.0b1pre

downloading same file should rename the downloaded item as:
file.ext
file(2).ext
file(3).ext 
see photo attached

Reproducible: Always

Steps to Reproduce:
1.start downloading same file multiple times

I have tried with pdf and also with ubuntu iso
Actual Results:  
the files should be renamed using (2), (3) after the name

Expected Results:  
the files are not renamed

Comment 1

[Ioana Chiorean]

Attachment: same file no renaming

Comment 2

[Brad Lassey [:blassey] (use needinfo?)]

This worked for me two weeks ago, if its regressed, its recent

Comment 3

[Ioana Chiorean]

still happening on Android nightly build 9/19  device Motorola Droid 2.2
Build Identifier: Mozilla /5.0 (Android;Linux armv7l;
rv:2.0b7pre)Gecko/20100919 Firefox/4.0b7pre Fennec /2.0b1pre

Comment 4

[ahoza]

Reproducible on N900 with the latest build:
Mozilla/5.0 (Maemo; Linux armv7l; rv:2.0b7pre) Gecko/20100927 Firefox/4.0b7pre Fennec/2.0b1pre
Changing Platform

Comment 5

[Wesley Johnston (:wesj)]

The download manager UI isn't a reliable indicator of the filename. Because we're using helper apps right now, the downloads are stored with a random filename during download. When the download is finished, the download is renamed and given a number at the end (but I doubt that filename is shown in the download manager either).

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Create unique name earlier

This seems like the "right" way to do this. This moves the creation of the unique filename up earlier in the download process. I'm asking you for review crowder, because I have no idea who else is to ask.

This seems necessary if anyone is going to access files opened with helper apps through the download manager.

Comment 7

[[e-mail]]

Comment on attachment 481100 [details] [diff] [review]
Create unique name earlier

>-      // Make sure the suggested name is unique since in this case we don't
>-      // have a file name that was guaranteed to be unique by going through
>-      // the File Save dialog

What about the case where you go through the File/Save dialog and choose the name of an existing file? Presumably the expected behavior is for that file to be overwritten. If you create a unique name earlier on, will it break that behavior?

Comment 8

[Wesley Johnston (:wesj)]

That should run through a different code path:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2245

Both paths will eventually call ExecuteDesiredAction:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2023

where the file is actually moved, but they're cased so that CreateUnique is only called for items that are being opened. I probably should have just killed off that entire check, and moved the "cancel if we can't create a unique file" stuff back into LaunchWithApplication. Revised patch coming up...

Comment 9

[Wesley Johnston (:wesj)]

Attachment: Fix v2

Comment 10

[[e-mail]]

Comment on attachment 481331 [details] [diff] [review]
Fix v2

r=dwitte

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mozilla-central/rev/95168409b1e4

Comment 12

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Verified:
Mozilla/5.0 (Maemo;Linux armv71; rv:2.0b8pre) Gecko/20101026 Firefox/4.0b8pre Fennec/4.0b2pre
Mozilla/5.0 (Android; Linux armv71; rv2.0b8pre) Gecko/20101026 Firefox/4.0b8pre Fennec/4.0b2pre

Comment 13

[Shawn Wilsher :sdwilsh]

This change did not meet the proper review requirements.  It's in the docshell module (https://wiki.mozilla.org/Modules/All#docshell) and neither the owner or a peer is cc'd to the bug, and it's filed in a component I suspect none of them watch.

Can we please make an effort to get proper review on changes like this in the future (and to put them in the right component so people who care about changes to these files can actually see them)?  This code is very delicate and poorly tested (and this patch *should* have had an automated test land with it since it is entirely testable).

I wouldn't care so much if this was fennec-only code, but it's shared with Firefox so you risk breaking more than just fennec.

Comment 14

[Shawn Wilsher :sdwilsh]

This patch actually changed behavior subtly.  We used to only create the final target file when we were done, but now we do it early in the download process (nsExternalAppHandler::LaunchWithApplication is deceptively named, but does have a comment explaining when it is called).  As a result, the final destination (mFinalFileDestination) will be kept around in the case of the user cancelling the download, or if an error occurs.  At the very least, we need to clean that file up.

Additionally, this patch doesn't even touch nsDownload::ExecuteDesiredAction, which does the same things this does.  It's hit in the cases where the user pauses a download, and then resumes it.  Not sure if fennec support that, but Firefox certain does, and now the behaviors in the two cases are subtly different.

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 481331 [details] [diff] [review]
Fix v2

Per comment 14

Comment 16

[Shawn Wilsher :sdwilsh]

Can you guys get a fix for this?

Comment 17

[Wesley Johnston (:wesj)]

Working on it.

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> Working on it.
Please add some tests too.  I can do a first-pass review, but ultimately, bz is going to have to review it.  Thanks for fixing this!

Comment 19

[Brian Crowder]

Fix as a follow-up would be better hygiene, unless this got backed out?  If it got backed out, what was the hg-rev of the backout?

Comment 20

[[e-mail]]

It hasn't been backed out, but if Shawn thinks the problems are serious enough then we should do so immediately and mark this a beta3 blocker. (Regardless, it should be a beta8/beta3 blocker, no?)

If there are e10s bits to the patch, I can review those, but it doesn't look like there will be.

Comment 21

[[e-mail]]

Er, I meant beta8 blocker for the case where this doesn't get backed out.

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Fix as a follow-up would be better hygiene, unless this got backed out?  If it
> got backed out, what was the hg-rev of the backout?
Wasn't backed out, but I'm being flexible and letting you fix issues instead of the backout.  This patch landed without proper peer review and without tests, so we could just back it out, but I'm fine as long as we get things fixed.

Comment 23

[[e-mail]]

Would you like to mark as a beta8 blocker, then?

Comment 24

[[e-mail]]

To be clear, beta 8 freeze is scheduled for Monday the 22nd. Wes, do you think we can get this by then?

Comment 25

[Wesley Johnston (:wesj)]

I think I can do that. I'll hollar if it looks like its going to be a problem.

Comment 26

[Wesley Johnston (:wesj)]

Attachment: Tests

Tests for downloading two files with the same name, and to make sure things are deleted when we cancel a download. I'm not exactly sure what bugs to look for arising from the DownloadManager::ExecuteDesiredAction function. At worst it seems like it might result in generating a third unique filename, but I'll have to dig more to try and make it happen.

Comment 27

[Wesley Johnston (:wesj)]

Attachment: Cleanup

Fixes for cancelling downloads, and for making the behavior in nsDownload::ExecuteDesiredAction more closely match what we're doing in nsExternalHelperService. We only delete the files in nsDownloadManager, which seems to be inline with the behavior in:

http://mxr.mozilla.org/mozilla-central/source/uriloader/exthandler/nsExternalHelperAppService.cpp#2474

which doesn't delete anything unless the user cancelled out of the "Save or open" dialog (in which case, we don't even have a final-target file).

Comment 28

[Shawn Wilsher :sdwilsh]

I won't be able to review this until tomorrow; sorry for the delay.

(In reply to comment #26)
> Tests for downloading two files with the same name, and to make sure things are
> deleted when we cancel a download. I'm not exactly sure what bugs to look for
> arising from the DownloadManager::ExecuteDesiredAction function. At worst it
> seems like it might result in generating a third unique filename, but I'll have
> to dig more to try and make it happen.
I'm mostly worried about this case:
1) complete a download with filename X
2) start a download with filename X (so that we'd end up creating a new unique name for it)
3) pause the download (via nsIDownloadManager::pauseDownload)
4) resume the download
That's the only way that I'm thinking would be broken, but I haven't looked super closely at that code path yet.

Comment 29

[Shawn Wilsher :sdwilsh]

Comment on attachment 491119 [details] [diff] [review]
Tests

>+++ b/toolkit/components/downloads/test/unit/test_bug_593815.js
use public domain license block for the boilerplate please:
http://www.mozilla.org/MPL/boilerplate-1.1/
Additionally, I'd prefer a descriptive filename and not a bug number (when it fails you have to go to the bug or read the test to figure out what it was actually testing).

>+// Dynamically generates a classID for our component, registers it to mask
>+// the existing component, and stored the masked components classID to be
>+// restored later, when we unregister.
>+function registerTemporaryComponent(comp)
>+{
>+  let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
>+  if (!comp.prototype.classID) {
>+    let uuidgen = Cc["@mozilla.org/uuid-generator;1"].getService(Ci.nsIUUIDGenerator);
>+    comp.prototype.classID = uuidgen.generateUUID();
>+  }
>+  comp.prototype.maskedClassID = Components.ID(Cc[comp.prototype.contractID].number);
>+  if (!comp.prototype.factory)
>+    comp.prototype.factory = getFactory(comp);
>+  registrar.registerFactory(comp.prototype.classID, "", comp.prototype.contractID, comp.prototype.factory);
>+}
>+
>+function unregisterTemporaryComponent(comp)
>+{
>+  let registrar = Components.manager.QueryInterface(Ci.nsIComponentRegistrar);
>+  registrar.unregisterFactory(comp.prototype.classID, comp.prototype.factory);
>+  registrar.registerFactory(comp.prototype.maskedClassID, "", comp.prototype.contractID, null);
>+}
I actually think you want to do a do_load_manifest here and have the components defined outside of this file (and you can use them for your other test file).

>+let DownloadListener = {
>+  onFinished: function(subject, topic, data) {
>+    let file = subject.QueryInterface(Ci.nsIDownload).targetFile;
>+    do_check_eq(file.leafName, this.set[1]);
>+  
>+    let fis = Cc["@mozilla.org/network/file-input-stream;1"].
>+                  createInstance(Ci.nsIFileInputStream);
nit: Cc and createInstance should line up

>+    fis.init(file, -1, -1, 0);
>+    var cstream = Cc["@mozilla.org/intl/converter-input-stream;1"].
>+                  createInstance(Ci.nsIConverterInputStream);
>+    cstream.init(fis, "UTF-8", 0, 0);
>+  
>+    let val = "";
>+    let (str = {}) {  
>+      let read = 0;
>+      do {   
>+        read = cstream.readString(0xffffffff, str);
>+        val += str.value;  
>+      } while (read != 0);  
>+    }  
>+    cstream.close();
You should be able to just use NetUtil.readInputStreamToString here

>+  observe: function (subject, topic, data) {
>+    this.onFinished(subject, topic, data);
>+  },
Not clear why you don't just put onFinished code here and get rid of it

>+  QueryInterface: function (iid) {
>+    if (iid.equals(Ci.nsIObserver) ||
>+        iid.equals(Ci.nsISupportsWeakReference) ||
>+        iid.equals(Ci.nsISupports))
>+      return this;
>+
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  }
Please use XPCOMUtils.generateQI here

>+function WindowContext() { }
>+WindowContext.prototype = {
>+  getInterface: function (iid) {
>+    if (iid.equals(Ci.nsIInterfaceRequestor) ||
>+        iid.equals(Ci.nsIURIContentListener) ||
>+        iid.equals(Ci.nsILoadGroup) ||
>+        iid.equals(Ci.nsIDocumentLoader) ||
>+        iid.equals(Ci.nsIDOMWindow))
>+      return this;
>+
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  },
nsIInterfaceRequestor shouldn't be in getInterface (but it should be in the QI, which you need to implement)
You also don't implement nsIDocumentLoader or nsIDOMWindow, so those shouldn't be here.

>+  /* nsIURIContentListener */
note: the more common style in toolkit is like this:
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/downloads/src/nsDownloadManagerUI.js#56

>+function HelperAppDlg() { }
note: this should be moved out and loaded with the manifest function I mentioned earlier (and please document it a bit better when you do this)

>+HelperAppDlg.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIHelperAppLauncherDialog]),
>+  contractID: "@mozilla.org/helperapplauncherdialog;1",
>+  show: function (launcher, ctx, reason) {
>+    launcher.MIMEInfo.preferredAction = Ci.nsIMIMEInfo.saveToDisk;//useHelperApp;
not sure what the comment at the end is about.  Either remove it, or make it explain more please

>+// Stolen from XPCOMUtils, since this handy function is not public there
>+function getFactory(comp)
>+{
>+  return {
>+    createInstance: function (outer, iid) {
>+      if (outer)
>+        throw Cr.NS_ERROR_NO_AGGREGATION;
>+      return (new comp()).QueryInterface(iid);
>+    }
>+  }
>+}
You shouldn't need this with the manifest stuff either

>+// Override the download-manager-ui to prevent anyone from trying to open
>+// a window.
>+function DownloadMgrUI() { }
>+DownloadMgrUI.prototype = {
>+  QueryInterface: XPCOMUtils.generateQI([Ci.nsIDownloadManagerUI]),
>+  contractID: "@mozilla.org/download-manager-ui;1",
>+  show: function (ir, aID, reason) { },
>+  visible: false,
>+  getAttention: function () { }
>+}
You can avoid this by just setting the preference browser.download.manager.showWhenStarting to false.

>+function runTestSet(set)
java-doc style comments please.  "set" is ambigious (and please prefix the pArameter with a).

>+  let uri = Services.io.newURI("http://localhost:4444" + set[0], null, null);
>+  let channel = Services.io.newChannelFromURI(uri);
Just do NetUtil.newChannel("http://localhost:4444" + set[0]);

>+let tests = [
>+  [ "/test1.html", "test.txt",   "Test data 1" ],
>+  [ "/test2.html", "test-1.txt", "Test data 2" ]
>+];
>+
>+function run_test() {
>+  registerTemporaryComponent(HelperAppDlg);
>+  registerTemporaryComponent(DownloadMgrUI);
>+  DownloadListener.init();
>+
>+  httpserver = new nsHttpServer();
>+  httpserver.start(4444);
>+  do_test_pending();
>+
>+  for each (set in tests)
>+    httpserver.registerPathHandler(set[0], getResponse(set));
>+
>+  runNextTest();
>+}
It's not entirely clear what this test is doing though, so it'd be helpful to add some comments in run_test to explain what is going on.

>+++ b/toolkit/components/downloads/test/unit/test_bug_593815_2.js
ditto re: license block and file name

>+var downloadManager = Cc["@mozilla.org/download-manager;1"]
>+                      .getService(Ci.nsIDownloadManager);
We use dm in other tests here (I'm actually surprised to not see it in head_download_manager.js; feel free to add it there)

Most of the comments on the other test file also apply here.  Also, please add a test for what the behavior asked in comment 28.

Comment 30

[Shawn Wilsher :sdwilsh]

Comment on attachment 491120 [details] [diff] [review]
Cleanup

>+++ b/toolkit/components/downloads/src/nsDownloadManager.cpp
> nsresult
> nsDownload::OpenWithApplication()
> {
>   // First move the temporary file to the target location
>   nsCOMPtr<nsILocalFile> target;
>   nsresult rv = GetTargetFile(getter_AddRefs(target));
>   NS_ENSURE_SUCCESS(rv, rv);
> 
>-  // Make sure the suggested name is unique since in this case we don't
>-  // have a file name that was guaranteed to be unique by going through
>-  // the File Save dialog
>-  rv = target->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
>-  NS_ENSURE_SUCCESS(rv, rv);
>-
Doesn't this mean that if we end up downloading a file with the same name as something in the target folder, we overwrite it?

Comment 31

[Wesley Johnston (:wesj)]

I'll try to get something back up today.

> Doesn't this mean that if we end up downloading a file with the same name as
> something in the target folder, we overwrite it?

Unless I'm mistaken, the flow should be something like:

ExternalAppService::OnStartRequest - creates a temp file for data, and a target file (unique) for the final data. Adds these to the download manager.

DownloadManager::PauseDownload - cancels the download in the nsExternalAppHandler, but doesn't delete either of the files

DownloadManager::ResumeDownload - resumes the download, doesn't attempt to create new files. just uses the targets stored in the download

DownloadManager::OpenWithApplication - only hit if we have resumed a download (i.e. the ExternalAppService doesn't have any hold on the download) and moves the temp file into the target one, same as the ExternalAppService would have done if it was around. Previously it might have created a third unique file...

So the unique filename should be guaranteed at the beginning of the download... unless something has already started writing into the target file we shouldn't be overwriting anything.

If you cancel both files are deleted. When you restart the download it is run through nsIWebBrowserPersist which should result in the creation of new (unique) files.

Comment 32

[Wesley Johnston (:wesj)]

Attachment: Tests v2

Fixup for tests. Moved the remaining component into a manifest. Added pause/resume support and included the suggested test in the same file as the main test here (i.e. it downloads three files with the same name, and pauses one of the downloads). Cancel tests are also testing some pause->resume circumstances.

Comment 33

[Shawn Wilsher :sdwilsh]

(In reply to comment #32)
> Fixup for tests. Moved the remaining component into a manifest. Added
> pause/resume support and included the suggested test in the same file as the
> main test here (i.e. it downloads three files with the same name, and pauses
> one of the downloads). Cancel tests are also testing some pause->resume
> circumstances.
Awesome.  I'll review this tomorrow.

Comment 34

[Wesley Johnston (:wesj)]

Given that this review isn't done, and I need to get an additional review afterwards from bz, do you think it would be best to remove the blocking flag here and push this to beta 9? It should be ready to land as soon as the tree reopens after beta 8.

Comment 35

[Shawn Wilsher :sdwilsh]

(In reply to comment #34)
> Given that this review isn't done, and I need to get an additional review
> afterwards from bz, do you think it would be best to remove the blocking flag
> here and push this to beta 9? It should be ready to land as soon as the tree
> reopens after beta 8.
Sorry; other stuff came up on Friday, but it will be my first priority Monday.  bz - can you review this at this point?  I think I don't have any more comments about the exthandler stuff, and that's what you'd be reviewing.  I own the download manager code, so my review is sufficient there.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 491120 [details] [diff] [review]
Cleanup

r=me on the exthandler bit.

Comment 37

[Shawn Wilsher :sdwilsh]

Comment on attachment 491724 [details] [diff] [review]
Tests v2

>+++ b/toolkit/components/downloads/test/unit/test_cancel_download_files_removed.js
>+  // check that relevant cancel operations have been performed
>+  // currently this jus means removing the target file
nit: just

>+  observe: function (aSubject, aTopic, aData) {
>+    switch(aTopic) {
>+      case "dl-start" :
>+        // cancel, pause, or resume the download
>+        // depending on parameters in this.set
>+        let dl = aSubject.QueryInterface(Ci.nsIDownload);
>+        if (this.set.doPause)
>+          downloadManager.pauseDownload(dl.id);
>+        if (this.set.doResume)
>+          downloadManager.resumeDownload(dl.id);
nit: please brace all ifs, even if they only are on one line per https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Naming_and_Formatting_code

>+  QueryInterface: function (iid) {
>+    if (iid.equals(Ci.nsIObserver) ||
>+        iid.equals(Ci.nsISupportsWeakReference) ||
>+        iid.equals(Ci.nsISupports))
>+      return this;
>+
>+    throw Cr.NS_ERROR_NO_INTERFACE;
>+  }
XPCOMUtils.generateQI

r=sdwilsh

Comment 38

[Wesley Johnston (:wesj)]

Attachment: Tests v3

Nits addressed. Having a problem running these (with or without nits addressed) locally today. I suspect its a local problem, as they ran fine last week, and I'm clobbering to test again right now, but wanted to get this up.

Comment 39

[Wesley Johnston (:wesj)]

Attachment: Tests v4

Ahh. Found the problem. All tests pass.

Comment 40

[Wesley Johnston (:wesj)]

This is giving persistant oranges on Windows Debug builds:

http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1290471554.1290473891.29792.gz

TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\tryserver-win32-debug-unittest-xpcshell\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_bug_401582.js | test failed (with xpcshell return code: 3), see following log:
TEST-UNEXPECTED-FAIL | e:\builds\moz2_slave\tryserver-win32-debug-unittest-xpcshell\build\xpcshell\tests\toolkit\components\downloads\test\unit\test_bug_409179.js | test failed (with xpcshell return code: 3), see following log:

I suspect it might have to do with defining downloadManager in the head for the tests. I'm going to try changing that and push to try again.

Comment 41

[Shawn Wilsher :sdwilsh]

(In reply to comment #40)
> I suspect it might have to do with defining downloadManager in the head for the
> tests. I'm going to try changing that and push to try again.
Given:
uncaught exception: [Exception... "Component returned failure code: 0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS) [nsILocalFile.copyTo]"  nsresult: "0x80520008 (NS_ERROR_FILE_ALREADY_EXISTS)"  location: "JS frame :: e:/builds/moz2_slave/tryserver-win32-debug-unittest-xpcshell/build/xpcshell/tests/toolkit/components/downloads/test/unit/head_download_manager.js :: importDownloadsFile :: line 92"  data: no]
Yes, that is the cause.  Just change it to to a lazy service getter with XPCOMUtils.defineLazyServiceGetter.

Comment 42

[Wesley Johnston (:wesj)]

Attachment: Tests Final

Passed try

Comment 43

[Wesley Johnston (:wesj)]

Attachment: Final Cleanup

Passed Try

Comment 44

[LSmith]

What a bunch of ****!

Comment 45

[Shawn Wilsher :sdwilsh]

(In reply to comment #44)
> What a bunch of crap!
Please try to adhere to bugzilla etiquette when posting.  Your post was neither useful nor constructive.
https://bugzilla.mozilla.org/page.cgi?id=etiquette.html

Comment 46

[Raul Nicolae Malea]

Working renamed same downloads for my minefield: Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101129 Firefox/4.0b8pre ID:20101129030317

about:buildconfig
Source

Built from http://hg.mozilla.org/mozilla-central/rev/5f9204fe5cd5
Build platform
target
i686-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
d:/mozilla-build/python25/python2.5.exe -O /e/builds/moz2_slave/mozilla-central-win32-nightly/build/build/cl.py cl 	14.00.50727.762 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
d:/mozilla-build/python25/python2.5.exe -O /e/builds/moz2_slave/mozilla-central-win32-nightly/build/build/cl.py cl 	14.00.50727.762 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
Configure arguments

--enable-application=browser --enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --enable-tests

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this ready to land?  It's not clear to me whether the patches are fully reviewed, and reviewers aren't noted in the commit messages inside the patches.  Do you have push access or do you need someone to push it for you?

Comment 48

[Wesley Johnston (:wesj)]

Ready to land, but I need someone to push for me (I thought checkin-needed would get someone to do that?). I'll update the reviewer names though. Sorry bout that.

Comment 49

[Wesley Johnston (:wesj)]

Attachment: Tests Final v2

With reviewer names

Comment 50

[Shawn Wilsher :sdwilsh]

(In reply to comment #48)
> Ready to land, but I need someone to push for me (I thought checkin-needed
> would get someone to do that?). I'll update the reviewer names though. Sorry
> bout that.
Note: http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 51

[Wesley Johnston (:wesj)]

Attachment: Final Cleanup v2

Final with reviewer names

Comment 52

[Wesley Johnston (:wesj)]

Attachment: Tests Final v3

Arrgh. Wrong file last time.

Comment 53

[Raul Nicolae Malea]

Is really blocker for Firefox 4 beta8 this bug? 
Perhaps blocking only Fennec release(or Firefox mobile release).

In my Minefield version, renamed same files working correctly.

Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101130 Firefox/4.0b8pre ID:20101130030317
Source

Built from http://hg.mozilla.org/mozilla-central/rev/837d7b346a64
Build platform
target
i686-pc-mingw32
Build tools
Compiler 	Version 	Compiler flags
d:/mozilla-build/python25/python2.5.exe -O /e/builds/moz2_slave/mozilla-central-win32-nightly/build/build/cl.py cl 	14.00.50727.762 	-TC -nologo -W3 -Gy -Fdgenerated.pdb -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
d:/mozilla-build/python25/python2.5.exe -O /e/builds/moz2_slave/mozilla-central-win32-nightly/build/build/cl.py cl 	14.00.50727.762 	-GR- -TP -nologo -Zc:wchar_t- -W3 -Gy -Fdgenerated.pdb -wd4800 -DNDEBUG -DTRIMMED -Zi -Zi -UDEBUG -DNDEBUG -GL -wd4624 -wd4952 -O1
Configure arguments

--enable-application=browser --enable-update-channel=nightly --enable-update-packaging --enable-jemalloc --enable-tests

Comment 54

[Shawn Wilsher :sdwilsh]

(In reply to comment #53)
> In my Minefield version, renamed same files working correctly.
Part of this bug already landed, which is probably why.

Comment 55

[Doug Turner (:dougt)]

http://hg.mozilla.org/mozilla-central/rev/17cecf2c1824
http://hg.mozilla.org/mozilla-central/rev/bdd47e6e4f17

Comment 58

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

litmus case already placed in: https://litmus.mozilla.org/show_test.cgi?id=49293