See bug 674742 for the long discussion. COPY_FILE_ALLOW_DECRYPTED_DESTINATION isn't supported by 2K so it causes failures in file copies. This in turn caused some data loss for mail files in tandem with mailnews handling of compaction. This patch needs some testing, and I'm not sure about the move file params split between 2k and os > 2k. Need to investigate.

Some cleanup and additional commenting. On the move params, we nixed MOVEFILE_WRITE_THROUGH for performance reasons, and replaced MOVEFILE_COPY_ALLOWED with our own copy / delete operation due to SMBV2 issues. I also touched up the remote path detection. IsRemoteFilePath shouldn't be failing, but if it does, I don't think we should fall through to the slower COPY_FILE_NO_BUFFERING operation. This needs to go through try first before I kick off a review.

Try run for 11206962781f is complete. Detailed breakdown of the results available here: https://tbpl.mozilla.org/?tree=Try&rev=11206962781f Results (out of 3 total builds): failure: 3 Builds (or logs if builds failed) available at: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jmathies@mozilla.com-11206962781f

Pushed again without the typo.

Why does it matter at all? I thoght we have actively removed a support for Win2k (bug 699247 and bug 730211). Nightly and Daily will no longer even run on Win2k.

I've even removed a security check preventing buffer overflow in bug 699247. https://hg.mozilla.org/mozilla-central/rev/3aa3c980b5ec#l18.36

Comment on attachment 658170 [details] [diff] [review] patch v.2 How did you test the patch on Win2k? Is this patch made for ESR10?

Comment on attachment 658170 [details] [diff] [review] patch v.2 + DWORD dwVersion = GetVersion(); + DWORD dwMajorVersion = (DWORD)(LOBYTE(LOWORD(dwVersion))); + bool is2K = false, isVistaAndUp = false; + if (dwMajorVersion <= 5) { + is2K = true; + } else if (dwMajorVersion > 5) { + isVistaAndUp = true; + } Actually is2K will become true on WinXP (version 5.1, major version is 5) and Win2k3 (version 5.2). Either the check or the flag name is wrong.

(In reply to Masatoshi Kimura [:emk] from comment #4) > Why does it matter at all? I thoght we have actively removed a support for > Win2k (bug 699247 and bug 730211). Nightly and Daily will no longer even run > on Win2k. I'm not sure if it does. But obviously people are still using some mozilla software on 2K, otherwise bug 674742 never would have been opened.

(In reply to Masatoshi Kimura [:emk] from comment #7) > Comment on attachment 658170 [details] [diff] [review] > patch v.2 > > + DWORD dwVersion = GetVersion(); > + DWORD dwMajorVersion = (DWORD)(LOBYTE(LOWORD(dwVersion))); > + bool is2K = false, isVistaAndUp = false; > + if (dwMajorVersion <= 5) { > + is2K = true; > + } else if (dwMajorVersion > 5) { > + isVistaAndUp = true; > + } > Actually is2K will become true on WinXP (version 5.1, major version is 5) > and Win2k3 (version 5.2). Either the check or the flag name is wrong. Oy, good catch. That version test is broken.

(In reply to Masatoshi Kimura [:emk] from comment #6) > Comment on attachment 658170 [details] [diff] [review] > patch v.2 > > How did you test the patch on Win2k? Is this patch made for ESR10? I haven't tested on 2K at all.

(In reply to Jim Mathies [:jimm] from comment #8) > I'm not sure if it does. But obviously people are still using some mozilla > software on 2K, otherwise bug 674742 never would have been opened. At the point of bug 674742 was opened, Win2k was still supported. Now it isn't anymore. Note that the bug number 674742 is smaller than 699247. No users would still be using latest Firefox on Win2k because it will not even start.

(In reply to Masatoshi Kimura [:emk] from comment #11) > (In reply to Jim Mathies [:jimm] from comment #8) > > I'm not sure if it does. But obviously people are still using some mozilla > > software on 2K, otherwise bug 674742 never would have been opened. > At the point of bug 674742 was opened, Win2k was still supported. Now it > isn't anymore. Note that the bug number 674742 is smaller than 699247. No > users would still be using latest Firefox on Win2k because it will not even > start. Ok, I'm find if we want to resolve this WONTFIX. I was just trying to smooth over the situation in bug 674742.

(Masatoshi comment #4) > Why does it matter at all? I thoght we have actively removed a support for > Win2k You should do, what you should do from the beginning. At that time W2K was valid for you. You did a very lousy job back then. It is not something minor. You will see my explanation below. Now you try to justify it by conditions, which were not present then. Or worse you try to cover this up. As if nothing happened, and nobody is guilty of "Dereliction of duties". See a section by same name in my attachment to bug 674742, named "Description ...". In that bug, and in the attachment there, and in other related bugs I mentioned that you introduced one bug (a supposed patch). Then you laid a few more bugs over it (other supposed patches). Splitting one bug into several is wrong, and cumbersome. You force me to reiterate, what in bug 674742, and bug 199473. I already explained: you should rely on the return value of the function, rather than on the OS. Because in theory CopyFileExW may fail to accept any flag in any version of OS. But the patch for bug 199473 never checked for anything (not version, not return value). It had a few more bugs. I mentioned all that already. Splitting bugs is really bad. The patch for bug 199473 is a bug in itself. It should be patched itself, or reverted (removed) at the time of conception 2 years ago. That's how late you are. It is also a bad code anyway. This bug report is actually a continuation of bug 199473. I suggested to reopen bug 199473. But Bugzilla ignored the request. Instead you split the issue into a separate report. This distracts from abuses, and my revelations in this bug 674742, and in bug 199473. Instead of making a patch for bug 199473, you introduced one more bug. What's more, you failed to complete the patch itself: you failed to introduce a flag of compaction for both cases "move" and "copy". You introduced it only for 1 case - "move". The code is bad anyway. That's why you have to do what is long overdue: patch, or remove it. The patch for bug 199473 is bad code: it does not check for acceptance of bad flags by CopyFileExW. It simply sets the flag. The patch for bug 545650 is also bad, because instead of checking the return value for acceptance of flags, it simply checks the version of OS. What's more: you patched the OS, not Mozilla. Even worse: you never tested the patch. You simply committed it. See my comment in bug 199473#c19. That's why it is better to remove the patch for bug 199473 (the bug of encryption). At the time of introduction of this buggy patch for bug 199473, you did actively support W2K. So, you would have caught your own bug at the time of conception. But you failed to catch it, because you never tested it. Why keep the patch for bug 199473, which nobody tests, or uses? I am very sorry, that you split this into one more bug report. So you force me to repeat these things over, and over... > Nightly and Daily will no longer even run > on Win2k. Bug 199473 prevents the run, does not it? Why not remove its patch? (Masatoshi comment #6) > How did you test the patch on Win2k? See bug 674742. There is my patch. I did test it already. Jim dragged that my patch into this separate report. - For no reason. There's nothing here, that we could not do there. (Masatoshi comment #7) > Actually is2K will become true on WinXP If you were right, then makers of a patch for bug 545650 made a bogus work too. They simply coded: "if (dwMajorVersion > 5)" Maybe they did not test their patch either. (Masatoshi comment #11) > At the point of bug 674742 was opened, Win2k was still supported. You contradict yourself in comment #4. But you do so only in order to justify the bug. At the time of conception of your bug 199473 (the bug of encryption) you still did support W2K. Patch for bug 199473 was a bug in itself. You should never commit it in the first place. > Now it isn't anymore. Hurray! Lousy work rewarded! You still need to mend the broken thing. The least you would have to do is rebuild the last version, which would still work on old OS. So, people may at least use something. Remember that you still do offer old downloads. But I really do not see why not continue with W2K. Do better code, which relies on return values rather than version. Then the entire code would be robust. > users would still be using latest Firefox on Win2k because it will not even > start. Again, that's because of the bug of encryption (bug 199473), and other similar bugs. Isn't it so? (Jim comment #12) > Ok, I'm find if we want to resolve this WONTFIX. The patch for bug 199473 was a bug in itself. It is also not complete (bad flag of encryption not set for case of "copy"). The patches for bug 199473, and bug 545650 are bad code (wrong checks, or no checks at all). The patch fro bug 199473 is not even tested. The patch for bug 545650 is perhaps not tested too. Both patches patch OS, rather than Mozilla. You have to mend them, because you had to do so to start with at the time you made it. Better remove both patches completely. > I was just trying to smooth > over the situation in bug 674742. You were trying to do bureaucracy rather than solve anything. You also tried to make the grave situation at Mozilla appear less so. Split the bug 674742 into pieces, into separate reports to distract attention. You gave no valid reason why split the bug 674742, by creating this one. I gave my valid reasons why keep all in one. The fact that you force me to repeat myself here only proves that. We would avoid repetition, if bugs remained as one report. This bug is a continuation of bug 199473. You divert attention from it too. So, you should cancel it, and return to 674742. Or at least reopen bug 199473, and move the stuff to it.

I think the platform is not only "x86_64". It is any "x86".

(In reply to Andrew from comment #13) > You should do, what you should do from the beginning. At that time W2K was > valid for you. At the time we supported Win2K this may have been an issue. But our applications do not run on 2K anymore. So I'm not sure why we should waste developer resources on fixing bugs nobody can hit. The code in nsILocalFileWin works on all platforms we currently support. What software are you using? Can you post your build information? > Splitting one bug into several is wrong, and cumbersome. It is the way we do things. > You force me to reiterate, what in bug 674742, and bug 199473. I already > explained: you should rely on the return value of the function, rather than > on the OS. Because in theory CopyFileExW may fail to accept any flag in any > version of OS. How is this the case? Are we passing invalid flags to CopyFileExW? Please try to keep your responses short and to the point so we can expedite this bug quickly. > The patch for bug 199473 is a bug in itself. It should be patched itself, or > reverted (removed) at the time of conception 2 years ago. That's how late > you are. That code succeeds on all platforms we support does it not? COPY_FILE_ALLOW_DECRYPTED_DESTINATION is supported on XP and up. > > This bug report is actually a continuation of bug 199473. > I suggested to reopen bug 199473. But Bugzilla ignored the request. Instead > you split the issue into a separate report. > This distracts from abuses, and my revelations in this bug 674742, and in > bug 199473. I've marked this bug as blocking 199473. > Instead of making a patch for bug 199473, you introduced one more bug. > What's more, you failed to complete the patch itself: you failed to > introduce a flag of compaction for both cases "move" and "copy". You > introduced it only for 1 case - "move". I don't understand what you are referring to here. If you are talking about mailnews related bits, please keep that discussion to bug 674742. > What's more: you patched the OS, not Mozilla. We often "patch the os" in mozilla code. We have to to address os shortcomings and bugs that show up in our products. This is common practice. So there's no point in rehashing this.

BTW, we are not going to reopen bug 199473. Standard practice is you open new bugs for regressions and mark them blocking. Hence this bug.

(In reply to Andrew from comment #13) What do the word "you" mean in your comment? I had never even seen bug 199473. > Bug 199473 prevents the run, does not it? No, it doesn't. Even if bug 199473 is reverted, the latest Firefox will not run on Win2K.

Fixing version check per - http://msdn.microsoft.com/en-us/library/windows/desktop/ms724832%28v=vs.85%29.aspx

Even better - consolidate COPY_FILE_ALLOW_DECRYPTED_DESTINATION use into a single flag for both operations.

>+ bool is2K = false, isVistaAndUp = false; >+ if (dwMajorVersion == 5 && dwMinorVersion == 0) { >+ is2K = true; >+ } else if (dwMajorVersion > 5) { >+ isVistaAndUp = true; >+ } Is WinNT4 and Win9x treated as WinXP and Win2k3? (I know the current Firefox will not run on WinNT4 and Win9x, but it will not run on Win2K either.)

One last change, always use copy file operations on encrypted files for move operations on non-2k systems. This preserves past behavior implemented in bug 199473. Also I reversed the order of move vs. copy so we're not checking for (!move).

(In reply to Masatoshi Kimura [:emk] from comment #20) > >+ bool is2K = false, isVistaAndUp = false; > >+ if (dwMajorVersion == 5 && dwMinorVersion == 0) { > >+ is2K = true; > >+ } else if (dwMajorVersion > 5) { > >+ isVistaAndUp = true; > >+ } > Is WinNT4 and Win9x treated as WinXP and Win2k3? (I know the current Firefox > will not run on WinNT4 and Win9x, but it will not run on Win2K either.) I'm not going to add version checks for anything lower than 2k. :)

Comment on attachment 658657 [details] [diff] [review] patch This addresses a 2K bug which we are fixing out of the kindness of our hearts. When dealing with encrypted files, we copy the file and then delete the src using COPY_FILE_ALLOW_DECRYPTED_DESTINATION. This addressed problems in the download manager with encrypted temp directories. (bug 199473). However COPY_FILE_ALLOW_DECRYPTED_DESTINATION is not supported on 2K, so the copy operation fails. The only way someone would be able to leverage this fix would be to build an app locally for 2K since nothing we produce today will run on that os AFAIK. I took the opportunity to fix up the formatting of this code as well. It was quite a mess.

(In reply to Jim Mathies [:jimm] from comment #22) > I'm not going to add version checks for anything lower than 2k. :) I don't think we need a check for Win2k either in our tree. Could you separate the Win2k supporting part from other cleanups?

(In reply to Jim Mathies [:jimm] from comment #23) > The only way someone would be able to leverage this fix would be to build an > app locally for 2K since nothing we produce today will run on that os AFAIK. If users build the tree locally for Win2k, they will need many patches anyway (for example reverting bug 699247). They can revert bug 199473 (or apply the patch in this bug) along with other changes.

(In reply to Masatoshi Kimura [:emk] from comment #25) > (In reply to Jim Mathies [:jimm] from comment #23) > > The only way someone would be able to leverage this fix would be to build an > > app locally for 2K since nothing we produce today will run on that os AFAIK. > If users build the tree locally for Win2k, they will need many patches > anyway (for example reverting bug 699247). They can revert bug 199473 (or > apply the patch in this bug) along with other changes. Yeah we can do that. The patch is here so people like Andrew have it if they need it. On the flip side it's not a huge change so taking it on mc wouldn't be a big deal. Let's see what Neil has to say.

(Jim comment #15) > At the time we supported Win2K this may have been an issue. Sure. Wait another 7 years, and you wont have to patch anything for Windows at all. What happens, if Mozilla decides to support W2K again? Have you read my comment? The least what you have to do is to patch the last version, which still does start on old OS. Many other reasons too. And don't you agree? That the patch for bug 199473 is a very lousy code. It is not a mistake. It is a disaster. Very incompetent. I explained this in comment #13. > So I'm not sure why we should waste > developer resources Have you read my comment #13? Bad code, even on Win 5.1, 6, 7, and so on. People lost lots of mail. Nobody is responsible? I agree with you. Why do you waste you time on this? I really would like you to discard the patch for bug 199473. Then let the lousy maker of it to make the patch anew. Let the culprit clean up after himself. If he does not want to make a new patch, then why bother? Why cling to this piece of rubbish? Nobody even has checked whether bug 199473, and bug 545650 still exist in the OS. Nobody checked that. Neither now, nor while they were making the patches. And nobody tested them. You simply committed them. Because, had you tested them, at least the patch for bug 199473 would have failed. And you are talking about rules? > on fixing bugs nobody can hit. So thought Steve - the maker of the buggy patch for 199473. The maker of bug 674742 (destruction of mail) also thought so. Make nice code. It would work better for all. > What software are you using? > Can you post your build information? Why? Are we talking about bad code? Or are we talking who is likely to suffer? The former is essential. The latter is circumstantial, hence not of great interest. > It is the way we do things. Mozilla is stagnant. I don't see why you can't reopen. Especially that you did not discard the patch for bug 199473. You try to continue it. In order to continue it is logical to reopen. > How is this the case? > Are we passing invalid flags to CopyFileExW? Absolutely. Otherwise there would be no regression. Just some versions of CopyFileEX do accept them, and some don't. Your code never tries to distinguish between a bad failure to copy, and a benign failure of flags. > Please try to keep your responses short and to the point Why? You barely read anything anyway. I exposed this your trait in bug 674742#c111. > so we can expedite this > bug quickly. Look at my attachment to bug 674742, named "Description ...". There I propose a "conservative" patch. It retains your bad flag. It does so in a very efficient manner. It corrects the flags only on failure. Not every time. If tries CopyFileExW. If it fails because of flags, it simply tries to use MoveFileExW. Although, I would prefer to to reverse the order: first try what should work, without the need for extra flags. Namely: MoveFileExW. And try a flag only if it fails. That way, the patch would turn itself off, if the OS is patched. > That code succeeds on all platforms we support does it not? How do you know? You never tested it. If you did, it would fail 2 years ago, at the time, when you committed the patch for bug 199473. Before making the patch Steve did not test whether the bug 199473 still exists. After making the patch nobody tested it either. Otherwise it would fail. And you are telling me that you do test all patches?.. This brings into doubt your entire system. Because there no system. Anyone can break and bypass anything. In bug 674742#c97 I already explained that "Bugzilla does not know" who is testing what, and how. > COPY_FILE_ALLOW_DECRYPTED_DESTINATION is supported on XP and up. The fact that XP does not fail is just one small luck. I repeat: the code is bad in many respects. I repeated many times: in theory any version of CopyFileExW may reject any flag. Version of the OS is not necessarily an indicator. Checking the result of a function is a better practice. Checks based on assumptions are bad. This code makes no checks at all. > I've marked this bug as blocking 199473. There's nothing blocking in it. Quite the reverse: bug 199473 was never tested, and never proven. It blocs your bug. Not the other way. So any work here is a fuss based on nothing. Not even on assumptions of existence of a bug, or its effects. You work purely on rearrangement of code. Not on any real issue. > I don't understand what you are referring to here. I told you a history how a patches for bug 199473, and 545650 came on top of each other. Patch for bug 199473 contains not 1, but at least 2 bugs. Later patch for bug 545650 failed to sport this. Makers of both patches failed to explore alternatives, and find real causes of the bug. Makers of both patches seem to have never tested neither the existence of bugs, nor the patches. Steve in bug 199473 certainly did not test anything. He just wanted to do something. I suspect that bug 545650 never existed. It may be a result of bad code elsewhere in Mozilla, which you never identified. But according to you bad code is good, as long as it works. It is doubly good, if it creates a need for more patches. You keep always busy. No time for real bugs. > We often "patch the os" in mozilla code. Bad practice. Report a bug to Microsoft. They love fixing them. But I suspect that guys at MS would find that there's no bug. Instead they would find a bug in Mozilla: some lazy, incompetent programmer. That is why you shy away from reports to MS. You avoid exposure. (Jim comment #16) > BTW, we are not going to reopen bug 199473 I have seen bugs reopened. For regression you do not make a new patch to replace the buggy one. Your rules say that you completely remove the bad patch. Then, if anyone wants, one may start making the patch anew. What you are doing is a mess. You know nothing about the bug 199473. You simply take the patch, and patch it so that the code works as before your change, just with the advantage of neat checks. If you had started the patch for bug 199473 anew, first you would have to test if the bug is actually there. Then you would have to look for solutions. Maybe using CopyFileEx, instead of MoveFileEx is not the only solution. You would have to explore several solutions, test, and discuss them. Only after that you make a new patch for bug 199473. Steve failed to do all of that too.

Comment on attachment 658657 [details] [diff] [review] patch I think I might try setting up a VM with Windows 2000 file encryption just to get a feel for how it works. >+ bool is2K = false, isVistaAndUp = false; >+ if (dwMajorVersion == 5 && dwMinorVersion == 0) { >+ is2K = true; >+ } else if (dwMajorVersion > 5) { >+ isVistaAndUp = true; >+ } [I don't like this way of setting flags. I'd just write something like bool isVistaAndUp = dwMajorVersion > 5; ] >+ bool path1Remote, path2Remote; >+ if (isVistaAndUp && >+ (IsRemoteFilePath(filePath.get(), path1Remote) && >+ IsRemoteFilePath(destPath.get(), path2Remote)) && >+ path1Remote || path2Remote) { I don't think your precedence is right here. It might be worth tweaking the condition to make it clearer what you're doing: if (isVistaAndUp && ((IsRemoteFilePath(filePath.get(), path1Remote) && path1Remote) || (IsRemoteFilePath(destPath.get(), path2Remote) && path2Remote)) { [extra parentheses added for readability] >+ if (encryptStatus == FILE_IS_ENCRYPTED && !is2K) { [encryptStatus is undefined if !is2K; I don't think this gives the optimiser enough rope to hang you but it would be better the other way around.]

Updated per comments.

(Masatoshi comment #20) > Is WinNT4 and Win9x treated as WinXP and Win2k3? Perhaps you should start by listing numbers, corresponding to OS. Unless you know them, the code is a mist. > will not run on WinNT4 and Win9x There is a difference in why they would not run. W98 would not run, because of different functions. WNT - because of flags. Mozilla replaced functions, like CopyFile with new versions, like CopyFileEx. W9x does not have the new functions. WNT - does. So, it would be feasible to make Mozilla run on NT. In fact all current versions of Windows are actually NT. This includes W2K, WXP, etc. They are all very related. This is one more reason to support W2K. Support for W9x would be difficult to return to. This would require not only change of functions, but also in more programming (not just flags). That's because new versions of the functions work better in WNT. The worth of that W9x is another matter. By the way, bug 199473, and bug 545650 are supposedly present (without proof) only in the supposedly better, newer OS. This is one more reason to keep support for the old good W2K.

(In reply to Andrew from comment #30) > So, it would be feasible to make Mozilla run on NT. Mozilla doesn't support any OS below XP. This decisions has been hashed over many many times. > This is one more reason to keep support for the old good W2K. If you would like to bring this discussion forward to the community I'd suggest posting to our platform newsgroup - https://groups.google.com/forum/?fromgroups#!forum/mozilla.dev.platform You are also free to work on this yourself. You can even fork the entire code base if you wish.

I will not accept a patch for this bug in the main codebase. We don't test Win2k and this would give the illusion of support.

You ignored my argument that: - bug 199473 is not complete anyway (applies only to case of "move"). - bug 199473 is never tested. - Patch for bug 199473 is never tested. Same perhaps goes for bug 545650. Revert them. Let their authors, and culprits reopen, and rework the entire patches, "if they wish", as you say. You don't need to test W2K. Just compile it, and state so. You did not test the mentioned patches anyway. Why bother about tests now?