Ever since bug 471962 landed: { TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | Timed out } Yet again: "You are not authorized to access bug #471962." :-<

Serge, I cced you on that bug for now.

[Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090318 SeaMonkey/2.0b1pre] (experimental/_m-c_, home, optim default) (W2Ksp4) (http://hg.mozilla.org/mozilla-central/rev/6f21cfb3a12d +http://hg.mozilla.org/comm-central/rev/...) Error Console reports: { downloadManager.addListener is not a function 233 downloadManager.addListener(downloadListener); }

(In reply to comment #2) downloadManager is a "xpconnect wrapped nsIDownloadManager", which should be fine.

This patch might fix the problem with the download manager on SeaMonkey, since it bypasses the download component completely. If everything is OK, after applying the patch the test should fail, but not time out.

(In reply to comment #4) > after applying the patch the test should fail, but not time out. Confirming with my local SeaMonkey: no more error nor timeout, fails with "The saved inner frame does not contain outer POST data" :->

I think we should just wrap that test in an SUITE_USING_XPFE_DM... the seamonkey folks are working to start using the toolkit download manager, so once that's complete the test should pass fine, and until then, we can just avoid running it for SeaMonkey.

It doesn't hurt to use a mock object here though, and really that's a more ideal thing with unit testing. You want the test to depend on as little behaviors of other code as possible.

(In reply to comment #7) > You want the test to depend on as little behaviors of other code as possible. Arguably that's what using a download listener does - it abstracts away some of the underlying implementation details and simplifies the test considerably. Given our need to maintain compatibility for addons that are also using this interface, I think it's unlikely that the dependency will cause a significant maintenance burden. Granted, that patch's dependencies are probably just as frozen as the download manager interfaces, but all else being equal it seems like we should go with the simpler test.

(In reply to comment #7) > You want the test to depend on as little behaviors of other code as possible. That's why I took the time to write the transfer mock object in the first place, as soon as I realized the availability of this technique :-) I'm more concerned with the download manager UI changing its behavior over time than the web progress listener being modified. Moreover, the download manager is just relying on a progress listener itself. Of course, any change in how the download manager UI works would not affect the download listener interface, so the test would still succeed, but maybe not as cleanly - the two preferences that now we set to keep the UI clean maybe will not be enough in the future. By using the download manager we are also making the implicit assumption that our download is the only one being run in that exact moment. This is quite a fair assumption for a test case, but no such assumption is made if we use the mock object. The amount of code required for using a download listener properly would be somewhat bigger if we didn't make this assumption. As far as simplicity goes, I wrote the code to be as clear as possible. If a reader understands the file picker mock object trick, then he'll certainly understand how the nsIWebProgressListener mock object works. A lot of code is also using web progress listeners to monitor page loads.

Comment on attachment 368046 [details] [diff] [review] Bypass the download manager using a mock transfer implementation r=sdwilsh, with one nit: don't have bracing around one line nits

(In reply to comment #10) > (From update of attachment 368046 [details] [diff] [review]) > r=sdwilsh, with one nit: > don't have bracing around one line nits Fixed.

Comment on attachment 368100 [details] [diff] [review] Bypass the download manager using a mock transfer implementation [Checkin: Comment 13 & 14] Don't need to request review from me again - I already gave it.

Comment on attachment 368100 [details] [diff] [review] Bypass the download manager using a mock transfer implementation [Checkin: Comment 13 & 14] http://hg.mozilla.org/mozilla-central/rev/20344c0c4faa

Comment on attachment 368100 [details] [diff] [review] Bypass the download manager using a mock transfer implementation [Checkin: Comment 13 & 14] http://hg.mozilla.org/releases/mozilla-1.9.1/rev/98164cf65618 *** New error is as expected: { http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1237563303.1237570410.26408.gz MacOSX 10.4 comm-central dep unit test on 2009/03/20 08:35:03 TEST-UNEXPECTED-FAIL | chrome://mochikit/content/browser/toolkit/content/tests/browser/browser_bug471962.js | The saved inner frame does not contain outer POST data }

Per comment 6, until bug 371157 is resolved, for 1.9.2 and 1.9.1.

(In reply to comment #15) > Per comment 6, until bug 371157 is resolved, for 1.9.2 and 1.9.1. Bug 381157 !

That fix is wrong, the test is completely right in that it shows we actually have a security bug there. Callek promised to look into fixing the actual bug soon, which is independent of what download manager implementation we are using.

(In reply to comment #17) Did he ? Iiuc his bug 471962 comment 64, Callek will do SM 1.1.x independently, not 1.9.x.

Comment on attachment 368749 [details] [diff] [review] (Bv1) Add |ifndef SUITE_USING_XPFE_DM| KaiRo is right (but all my comments to him were not public) I told KaiRo I'd look into our end of this asap as I don't like our boxen being orange any more than you do. I'm ok with this landing as is, (if a toolkit peer is ok with it, and would rs+ my removal of the flag when xpfe/ is fixed). r- this patch for the fact that fixing this is better, as well as the fact that I am not a toolkit peer.

Comment on attachment 368749 [details] [diff] [review] (Bv1) Add |ifndef SUITE_USING_XPFE_DM| hmm, on re-inspection of code, I'm actually against depending on suite DM here.. as the bug fix is orthogonal to suite DM (just the test required it at first). Anyway, I'm working on a fix.

Comment on attachment 368749 [details] [diff] [review] (Bv1) Add |ifndef SUITE_USING_XPFE_DM| Let me summarize: 1) I told Paolo that I would port the fix to SM. 2) Then Callek wrote not to and to use an ifdef in the meantime. 3) So I attached such a patch. 4) Now KaiRo flames me because I did not just guess that Callek had changed his mind without commenting. I'm really having no fun at these! :-(

V.Fixed, on my local SeaMonkey/1.9.2. verified1.9.1, per comment 14.

(In reply to comment #21) > (From update of attachment 368749 [details] [diff] [review]) > Let me summarize: > 1) I told Paolo that I would port the fix to SM. > 2) Then Callek wrote not to and to use an ifdef in the meantime. > 3) So I attached such a patch. > 4) Now KaiRo flames me because I did not just guess that Callek had changed his > mind without commenting. I am certain that KaiRo was not attempting to flame you. We do appreciate your help here! (p.s. I thought I did comment, and looked for said comment before I announced I changed my mind)

I just wanted to say thank you to anybody who's helped in working out this issue. I spent some time to catch up with the progress made while I was away, and I have to say that, while there have been some misunderstandings, things seem to be going on smoothly now. Justin is now working on bug 484621 (= fixing bug 471962 in SeaMonkey and Thunderbird too), which is the work Serge initially offered to do. I'm happy that in the end the road of porting the fix early was chosen, rather than just turning off the test and porting the fix later (actually, that solution was proposed just because the test initially depended on the Download Manager). The fix actually needed to be applied in six different places: * mozilla-central (HG) in "toolkit" folder * mozilla-1.9.1 (HG) in "toolkit" folder * Mozilla 1.9.0 (CVS trunk) in "toolkit" folder * comm-central (HG) in "suite" folder * Mozilla 1.8.1 (CVS branch) in "communicator" folder * Mozilla 1.8.1 (CVS branch) in "toolkit" folder The test, instead, can only work on mozilla-central and mozilla-1.9.1. The strange thing is that the mozilla-1.9.1 test suite, located in "toolkit/content/tests/browser", when run from a comm-central build is NOT testing the code in "toolkit/content/"! Serge filed bug 484616 to track why the "contentAreaUtils.js" code is duplicated between comm-central and mozilla-central, and try to remove this duplication. I'd be really grateful if anyone with some background information on what is the purpose (or the cause) of having the code duplicated can comment on bug 484616. I'm thinking about doing some work in this area, and I'm not enthusiastic about the idea of having to duplicate every change! I'd prefer to help in moving the code to a more sensible place, if it's not an unreasonable amount of work.

(In reply to comment #24) > The strange thing is that the mozilla-1.9.1 test suite, located in > "toolkit/content/tests/browser", when run from a comm-central build is > NOT testing the code in "toolkit/content/"! This is because it's testing whatever is _loaded_ into the browser window, and SeaMonkey is loading the suite/ version (right now) in its browser window while Firefox is loading the toolkit version. The reason might be that SeaMonkey might want to do some things differently than Firefox, but I think we probably should load the toolkit version and only change/override those things we want to have differently in SeaMonkey, if possible - but that's bug 484616 stuff and needs the input of Neil Rashbrook, who probably knows all this code best. Note that Thunderbird is not running those tests at all, as anything that requires a browser window cannot be tested well from a non-browser application and all mochitest-based test suites fall under that category.