Once bug 563318 lands Firefox will only run on Windows XP SP2 and higher, thus support for older versions of Windows can be removed.

Jacek, could you verify that this patch will not break MinGW builds badly?

Comment on attachment 594969 [details] [diff] [review] patch Review of attachment 594969 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, this is going to train wreck a ton of patches in my queue. :) Thanks for doing this. I'd like an sr from Neil on this as well. ::: gfx/thebes/gfxGDIFontList.cpp @@ +878,5 @@ > EOTFontStreamReader eotReader(aFontData, aLength, buffer, eotlen, > &overlayNameData); > > + ret = TTLoadEmbeddedFont(&fontRef, TTLOAD_PRIVATE, &privStatus, > + LICENSE_PREVIEWPRINT, &pulStatus, nit - might as well fix up this indentation. ::: netwerk/protocol/http/nsHttpHandler.cpp @@ +714,5 @@ > #elif defined _M_X64 || defined _M_AMD64 > format = WNT_BASE W64_PREFIX "; x64"; > #else > BOOL isWow64 = FALSE; > + if (!IsWow64Process(GetCurrentProcess(), &isWow64)) { "Windows Vista, Windows XP with SP2" - did we drop SP1 support as well? ::: widget/windows/TaskbarPreview.cpp @@ +40,5 @@ > * ***** END LICENSE BLOCK ***** */ > > #if MOZ_WINSDK_TARGETVER >= MOZ_NTDDI_WIN7 > +#undef _WIN32_WINNT > +#define _WIN32_WINNT 0x0601 Did we up _WIN32_WINNT in our build? I'm surprised this builds with this removed. ::: widget/windows/WinUtils.h @@ -60,5 @@ > > class WinUtils { > public: > enum WinVersion { > - WIN2K_VERSION = 0x500, Do we really want to remove this? We might want to leave the ability to detect it in our widget utility code.

Updated to tip and resolved review comments. > nit - might as well fix up this indentation. Done. > "Windows Vista, Windows XP with SP2" - did we drop SP1 support as well? Yes. VC10 doesn't support WinXP SP1 as well. See also bug 668574. > Did we up _WIN32_WINNT in our build? I'm surprised this builds with this removed. Ah, it is a wreckage of my experiment. Eventually, it was not required. Removed. > Do we really want to remove this? We might want to leave the ability to detect it in our widget utility code. It was not used at all from the start. https://mxr.mozilla.org/mozilla-central/search?string=WIN2K_VERSION If it's absolutely required to test unsupported versions, we can write the code as follows: WinUtils::GetWindowsVersion() < WinUtils::WINXP_VERSION

Comment on attachment 595004 [details] [diff] [review] patch v2 Looks good to me. There are some fixes required on mingw-w64 side (patches are on their way).

Comment on attachment 595004 [details] [diff] [review] patch v2 >- if (GetThemeDLL()) { Does anyone still use GetThemeDLL()? > NS_IMETHODIMP > nsLocalFile::Reveal() > { >- // make sure mResolvedPath is set >- nsresult rv = ResolveAndStat(); >- if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) >- return rv; >+ // make sure mResolvedPath is set >+ nsresult rv = ResolveAndStat(); >+ if (NS_FAILED(rv) && rv != NS_ERROR_FILE_NOT_FOUND) >+ return rv; > > bool isDirectory; >- nsresult rv = IsDirectory(&isDirectory); >+ rv = IsDirectory(&isDirectory); Actually IsDirectory calls ResolveAndStat (there is a patch to improve this to avoid calling GetFileAttributesEx) so you don't need to call it yourself.

Folded two patches. > Does anyone still use GetThemeDLL()? GetThemeDLL() is still used by nsWinGesture. https://hg.mozilla.org/mozilla-central/file/ccbb41b873cd/widget/windows/nsWinGesture.cpp#l100 I couldn't remove those GetProcAddress() calls because gesture functions are Win7 specific. > Actually IsDirectory calls ResolveAndStat (there is a patch to improve this > to avoid calling GetFileAttributesEx) so you don't need to call it yourself. Good catch. Fixed.

(In reply to Masatoshi Kimura [:emk] from comment #7) > > Does anyone still use GetThemeDLL()? > GetThemeDLL() is still used by nsWinGesture. > https://hg.mozilla.org/mozilla-central/file/ccbb41b873cd/widget/windows/ > nsWinGesture.cpp#l100 > I couldn't remove those GetProcAddress() calls because gesture functions are > Win7 specific. Plus, the nsUXThemeData GetProcAddress logic is coming back in bug 373266 due to Vista+ theme calls. Win8 also adds a number of theme calls we might be using.

(In reply to Jim Mathies [:jimm] from comment #10) > https://hg.mozilla.org/integration/mozilla-inbound/rev/6771bd53e267 This and the patch in bug 719983 are getting backed out due to Talos regressions. I'll post both individually to try for Talos runs to see which caused the problem.

Backed out of inbound along with bug 719983 at Jim's request, until the cause of a 30% WinXP Ts regression (https://groups.google.com/d/topic/mozilla.dev.tree-management/_yUe9mobQHA) is known. https://hg.mozilla.org/integration/mozilla-inbound/rev/07da69ba7e52

Two sets pushed to try config'd to report to each bug.

Hmm, Try results have ts_paint @ 590.47 so looks like it was this patch. Poking through it though I don't see anything obvious. https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=8426ecbadc6f Regression :( Ts, Paint increase 28.6% on XP Mozilla-Inbound-Non-PGO -------------------------------------------------------------------- Previous: avg 460.163 stddev 3.159 of 30 runs up to revision 1a345b043b47 New : avg 591.642 stddev 2.549 of 5 runs since revision f1acc52a59da Change : +131.479 (28.6% / z=41.617) Graph : http://mzl.la/xV3oaj

(In reply to Matheus Kerschbaum from comment #0) > Once bug 563318 lands Firefox will only run on Windows XP SP2 and higher, > thus support for older versions of Windows can be removed. There's a chance that that patch will be backed out. Why should this one land right now?

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

(In reply to Dão Gottwald [:dao] from comment #15) This patch will reduce code size, complexity, and extra CPU cycles. And not only bug 563318 dropped the support for WinXP SP1 or earlier but also other dependent bugs (for example 668574 and 723317). This is a purposeful decision. https://groups.google.com/d/topic/mozilla.dev.platform/pvy2IQ-LPWI/discussion https://groups.google.com/d/topic/mozilla.dev.apps.firefox/qwFkoDnkJeQ/discussion http://weblogs.mozillazine.org/asa/archives/2012/01/end_of_firefox_win2k.html > There's a chance that that patch will be backed out. We can back out this patch either if perchance. That said, we couldn't land this patch until we found what caused the perf regression.

Dão is referring to the crash stack issues, that if not resolved soon would mean reverting bug 563318 temporarily: https://groups.google.com/d/topic/mozilla.dev.platform/k4sRq9tV3RU/discussion

How can I run ts_paint on the tryserver? It seems to be disabled by the changeset <http://hg.mozilla.org/build/buildbot-configs/rev/6dddc55c434a>.

(In reply to Masatoshi Kimura [:emk] from comment #19) > How can I run ts_paint on the tryserver? > It seems to be disabled by the changeset > <http://hg.mozilla.org/build/buildbot-configs/rev/6dddc55c434a>. 'try: -b do -p win32 -u none -t chrome,nochrome,paint' runs most of the paint test.

Assuming bug 719983 makes to mc today, here's a touch up patch to byewin2k that fixes bustage.

updates.

(In reply to Jim Mathies [:jimm] from comment #20) > 'try: -b do -p win32 -u none -t chrome,nochrome,paint' > runs most of the paint test. No tests were executed at all with this option. https://tbpl.mozilla.org/?tree=Try&rev=6d2bd0475526 If I use '-t all', chrome.2 test is executed insted of chrome which contains ts_paint. I propose just land the patch again because: - ts_paint does no longer run on m-c and m-i either. - The "regression" is dubious. ts_paint was around 590 on Win7 even before the patch.

(In reply to Masatoshi Kimura [:emk] from comment #23) > (In reply to Jim Mathies [:jimm] from comment #20) > > 'try: -b do -p win32 -u none -t chrome,nochrome,paint' > > runs most of the paint test. > No tests were executed at all with this option. > https://tbpl.mozilla.org/?tree=Try&rev=6d2bd0475526 > If I use '-t all', chrome.2 test is executed insted of chrome which contains > ts_paint. > > I propose just land the patch again because: > - ts_paint does no longer run on m-c and m-i either. > - The "regression" is dubious. ts_paint was around 590 on Win7 even before > the patch. Did you try '-t all'? that worked for me here - https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=8426ecbadc6f (The TS,Paint test that regressed is displayed under the Talos chrome tests in the XP OPT build.) From that run I see a major perf regression - http://tinyurl.com/7jsoztv vs. mozilla-central: http://tinyurl.com/7qwsx3s You have approval so if you want to reland on mi to get talos test runs again you can, but if the xp opt regression shows up there's no way this will get merged to mc - here's the the inbound regression from the first landing: http://tinyurl.com/6qje9lt That doesn't seem very 'dubious'! :)

(In reply to Jim Mathies [:jimm] from comment #24) > Did you try '-t all'? that worked for me here - > https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=8426ecbadc6f I tried '-t all': https://tbpl.mozilla.org/?tree=Try&rev=8e7f43fd1a58 ts_paint did no longer run. Not only ts_paint, but also all tests belonging to chrome,nochrome did not run. > You have approval so if you want to reland on mi to get talos test runs > again you can, but if the xp opt regression shows up there's no way this > will get merged to mc - here's the the inbound regression from the first > landing: > > http://tinyurl.com/6qje9lt > > That doesn't seem very 'dubious'! :) I added Win7 graph: http://graphs.mozilla.org/graph.html#tests=[[83,63,1],[83,63,12]]&sel=1328666452046,1328852361012&displayrange=30&datatype=running Why Win7 result is consistently worse than WinXP? More importantly, ts_paint does not execute at all even on inbound since Feb 10, 2012 00:21. So it is unlikely to be backed out because of perf regression.

Comment on attachment 598463 [details] [diff] [review] rebased to tip We can't ignore this big of a regression, even if we're not running the tests anymore. bbondy offered to take a look next week, he's been working on perf lately.

jimm could you rebase to tip? GfxInfo.cpp changes are not applying:

that was fast, thanks :)

Pushed four patches to try with changes removed to try and isolate the area that caused this. https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=2bc9c051096f https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=eda8d8b4c8a5 https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=dbd8a05b3575 https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=923f6977a76b

Resubmitted and filed bug 729199 on running individual Talos tests.

I've submitted a few more patches with various areas removed. Thus far I can rule out the following areas: 1) theme related code in widget 2) fonts related code 3) netwerk related code

> I've submitted a few more patches with various areas removed. Thus far I can > rule out the following areas: > 3) netwerk related code Really? It looks like WinXP ts_paint went down to 480.32 when netwerk changes are reverted. https://tbpl.mozilla.org/?tree=Try&rev=fc1d6df25072

(In reply to Masatoshi Kimura [:emk] from comment #35) > > I've submitted a few more patches with various areas removed. Thus far I can > > rule out the following areas: > > 3) netwerk related code > Really? It looks like WinXP ts_paint went down to 480.32 when netwerk > changes are reverted. > https://tbpl.mozilla.org/?tree=Try&rev=fc1d6df25072 You're right, and those look pretty normal for that slave. http://graphs-old.mozilla.org/graph.html#tests=[[83,23,515]] I had ts_paint off mc at 418, so i missed it. I'll resubmit the netwerk changes for a couple more test runs.

recent pushes: stackwalk: https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=dad7e6ebc006 xpcom/localfile: https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=c3df1e252edc gfxinfo: https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=685ee54fbcc8 and new pushes: netwerk: https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=5f5ab7c54e6d https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=a13166b2cf5a

Ok, thanks for catching that Masatoshi. netwerk it is!

I've found this code has rather precarious timing issues on startup - http://mxr.mozilla.org/mozilla-central/source/netwerk/system/win32/nsNotifyAddrListener.cpp#578 CheckLinkStatus gets call when xpcom is starting up on the main thread. http://mxr.mozilla.org/mozilla-central/source/netwerk/system/win32/nsNotifyAddrListener.cpp#178 nsNotifyAddrListener::Run() executes simultaneously on a background thread. Before this patch landed, CheckLinkStatus() gets called first (on Win7) prior to InitIPHelperLibrary() in Run(), so CheckAdaptersAddresses(), CheckAdaptersInfo(), and CheckIPAddrTable() all return early due to uninitialized function pointers. We fall through to the default |mLinkUp = true; // I can't tell, so assume there's a link| and exit. Once the patch was applied, the GetAdaptersAddresses API call in CheckAdaptersAddresses() succeeded in getting called. According to MSDN: "GetAdaptersAddresses is implemented only as a synchronous function call. The GetAdaptersAddresses function requires a significant amount of network resources and time to complete since all of the low-level network interface tables must be traversed." There's also a comment in the file about this - // XXX this call is very expensive (~650 milliseconds), so we // don't want to call it synchronously. Instead, we just // start up assuming we have a network link, but we'll // report that the status isn't known. I think what we need to do here is bypass any real checks until after Run() is executed, or maybe just set mLinkUp to true on the first invocation of CheckLinkStatus. The second option might be best since Run() could get executed before CheckLinkStatus() on some systems. c'ing Makoto who recently worked on this code. Makoto - side note - if called on the main thread, the CoInitializeEx call you added back in bug 712243 will always fail with error 0x80010106 - "Cannot change thread mode after it is set." I think that's a bug since it prevents CheckAdaptersAddresses() from ever succeeding. Probably best to file that as a follow up though once we sort this out.

Actually, SendEventToUI() fires the runnable, which is called at the end of CheckLinkStatus()! So waiting until that fires should address the perf problem.

Cleaned up and better commented patch. I've removed the mStatusKnown check, there's no need for it. This should also get some dev doc treatment, MDN doesn't say this should be called on a background thread.

pushed to try: https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=92f91f90a91c

(In reply to Jim Mathies [:jimm] from comment #43) > pushed to try: > > https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=92f91f90a91c This build was successful and brought ts_paint back down to around 469.

https://hg.mozilla.org/integration/mozilla-inbound/rev/3aa3c980b5ec https://hg.mozilla.org/integration/mozilla-inbound/rev/5b0d34ea3eaa \o/

https://hg.mozilla.org/mozilla-central/rev/3aa3c980b5ec https://hg.mozilla.org/mozilla-central/rev/5b0d34ea3eaa

Docs updated: https://developer.mozilla.org/en/Firefox_13_for_developers#Changes_for_Mozilla_and_add-on_developers https://developer.mozilla.org/en/Gecko_FAQ#Which_platforms_does_Gecko_run_on.3F