Auto-renaming at download is not working

RESOLVED FIXED in mozilla2.0b8

Status

Core Graveyard
File Handling
RESOLVED FIXED
7 years ago
11 months ago

People

(Reporter: Ioana Chiorean, Assigned: wesj)

Tracking

Trunk
mozilla2.0b8
ARM
All
Bug Flags:
in-testsuite +
in-litmus +

Firefox Tracking Flags

(blocking2.0 beta8+, fennec2.0+)

Details

Attachments

(5 attachments, 8 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Reporter)

Comment 1

7 years ago
Created attachment 472408 [details]
same file no renaming

Updated

7 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0+
This worked for me two weeks ago, if its regressed, its recent
(Reporter)

Comment 3

7 years ago
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

7 years ago
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
OS: Other → All
Hardware: Other → ARM
(Assignee)

Comment 5

7 years ago
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).
(Assignee)

Comment 6

7 years ago
Created attachment 481100 [details] [diff] [review]
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.
Attachment #481100 - Flags: review?
Assignee: nobody → wjohnston
Attachment #481100 - Flags: review? → review?(crowderbt)
(Assignee)

Updated

7 years ago
Attachment #481100 - Flags: review?(crowderbt) → review?(dwitte)

Comment 7

7 years ago
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?
(Assignee)

Comment 8

7 years ago
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...
(Assignee)

Comment 9

7 years ago
Created attachment 481331 [details] [diff] [review]
Fix v2
Attachment #481100 - Attachment is obsolete: true
Attachment #481331 - Flags: review?(dwitte)
Attachment #481100 - Flags: review?(dwitte)

Comment 10

7 years ago
Comment on attachment 481331 [details] [diff] [review]
Fix v2

r=dwitte
Attachment #481331 - Flags: review?(dwitte) → review+
pushed:
http://hg.mozilla.org/mozilla-central/rev/95168409b1e4
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Flags: in-litmus?
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
Status: RESOLVED → VERIFIED
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.

Updated

7 years ago
Flags: in-litmus? → in-litmus?(nhirata.bugzilla)
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 on attachment 481331 [details] [diff] [review]
Fix v2

Per comment 14
Attachment #481331 - Flags: review-
Can you guys get a fix for this?
Status: VERIFIED → REOPENED
Component: General → File Handling
Product: Fennec → Core
QA Contact: general → file-handling
Resolution: FIXED → ---
(Assignee)

Comment 17

7 years ago
Working on it.
(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

7 years ago
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

7 years ago
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

7 years ago
Er, I meant beta8 blocker for the case where this doesn't get backed out.
(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

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

Updated

7 years ago
blocking2.0: --- → beta8+

Comment 24

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

Comment 25

7 years ago
I think I can do that. I'll hollar if it looks like its going to be a problem.
(Assignee)

Comment 26

7 years ago
Created attachment 491119 [details] [diff] [review]
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.
Attachment #491119 - Flags: review?(sdwilsh)
(Assignee)

Comment 27

7 years ago
Created attachment 491120 [details] [diff] [review]
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).
Attachment #491120 - Flags: review?(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 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.
Attachment #491119 - Flags: review?(sdwilsh) → review-
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?
(Assignee)

Comment 31

7 years ago
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.
(Assignee)

Comment 32

7 years ago
Created attachment 491724 [details] [diff] [review]
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.
Attachment #491119 - Attachment is obsolete: true
Attachment #491724 - Flags: review?(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.
(Assignee)

Comment 34

7 years ago
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.
(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 on attachment 491120 [details] [diff] [review]
Cleanup

r=me on the exthandler bit.
Attachment #491120 - Flags: review?(sdwilsh) → review+
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
Attachment #491724 - Flags: review?(sdwilsh) → review+
(Assignee)

Comment 38

7 years ago
Created attachment 492355 [details] [diff] [review]
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.
Attachment #491724 - Attachment is obsolete: true
Attachment #492355 - Flags: review+
(Assignee)

Comment 39

7 years ago
Created attachment 492370 [details] [diff] [review]
Tests v4

Ahh. Found the problem. All tests pass.
Attachment #492355 - Attachment is obsolete: true
Attachment #492370 - Flags: review+
(Assignee)

Comment 40

7 years ago
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.
(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.
(Assignee)

Comment 42

7 years ago
Created attachment 492612 [details] [diff] [review]
Tests Final

Passed try
Attachment #492370 - Attachment is obsolete: true
Attachment #492612 - Flags: review+
(Assignee)

Comment 43

7 years ago
Created attachment 492613 [details] [diff] [review]
Final Cleanup

Passed Try
Attachment #491120 - Attachment is obsolete: true
Attachment #492613 - Flags: review+
(Assignee)

Updated

7 years ago
Whiteboard: [checkin-needed]

Comment 44

7 years ago
What a bunch of ****!
(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
Status: REOPENED → ASSIGNED
Keywords: checkin-needed
Whiteboard: [checkin-needed]

Comment 46

7 years ago
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
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?
(Assignee)

Comment 48

7 years ago
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.
(Assignee)

Comment 49

7 years ago
Created attachment 494074 [details] [diff] [review]
Tests Final v2

With reviewer names
Attachment #492612 - Attachment is obsolete: true
Attachment #494074 - Flags: review+
(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
(Assignee)

Comment 51

7 years ago
Created attachment 494076 [details] [diff] [review]
Final Cleanup v2

Final with reviewer names
Attachment #494076 - Flags: superreview+
Attachment #494076 - Flags: review+
(Assignee)

Comment 52

7 years ago
Created attachment 494077 [details] [diff] [review]
Tests Final v3

Arrgh. Wrong file last time.
Attachment #494074 - Attachment is obsolete: true
Attachment #494077 - Flags: review+

Comment 53

7 years ago
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
(In reply to comment #53)
> In my Minefield version, renamed same files working correctly.
Part of this bug already landed, which is probably why.
http://hg.mozilla.org/mozilla-central/rev/17cecf2c1824
http://hg.mozilla.org/mozilla-central/rev/bdd47e6e4f17
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Flags: in-testsuite? → in-testsuite+
Keywords: checkin-needed
Target Milestone: --- → mozilla2.0b8

Updated

7 years ago
Duplicate of this bug: 301328
Duplicate of this bug: 446146
litmus case already placed in: https://litmus.mozilla.org/show_test.cgi?id=49293
Flags: in-litmus?(nhirata.bugzilla) → in-litmus+
Depends on: 793250
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.