699247 - Remove support for executing on Windows 2000

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Matheus Kerschbaum]

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

Comment 1

[Masatoshi Kimura [:emk]]

Attachment: patch

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

Comment 2

[Jim Mathies [:jimm]]

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.

Comment 3

[Masatoshi Kimura [:emk]]

Attachment: patch v2

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 4

[Jacek Caban]

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 5

[Masatoshi Kimura [:emk]]

Attachment: Remove more version checks

Comment 6

[[e-mail]]

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.

Comment 7

[Masatoshi Kimura [:emk]]

Attachment: patch for check in; r=jmathies, sr=neil

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.

Comment 8

[Jim Mathies [:jimm]]

(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.

Comment 9

[Masatoshi Kimura [:emk]]

https://tbpl.mozilla.org/?tree=Try&rev=e29ff3429d08

Comment 10

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6771bd53e267

Comment 11

[Jim Mathies [:jimm]]

(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.

Comment 12

[Ed Morley [:emorley]]

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

Comment 13

[Jim Mathies [:jimm]]

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

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[Dão Gottwald [:dao]]

(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?

Comment 16

[Mozilla RelEng Bot]

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/[e-mail]-8426ecbadc6f

Comment 17

[Masatoshi Kimura [:emk]]

(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.

Comment 18

[Ed Morley [:emorley]]

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

Comment 19

[Masatoshi Kimura [:emk]]

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>.

Comment 20

[Jim Mathies [:jimm]]

(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.

Comment 21

[Jim Mathies [:jimm]]

Attachment: bug 719983 fixes

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

Comment 22

[Jim Mathies [:jimm]]

Attachment: bug 719983 fixes

updates.

Comment 23

[Masatoshi Kimura [:emk]]

(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.

Comment 24

[Jim Mathies [:jimm]]

(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'! :)

Comment 25

[Masatoshi Kimura [:emk]]

(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 26

[Masatoshi Kimura [:emk]]

Attachment: rebased to tip

Comment 27

[Masatoshi Kimura [:emk]]

Attachment: rebased to tip

Comment 28

[Jim Mathies [:jimm]]

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.

Comment 29

[Brian R. Bondy [:bbondy]]

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

Comment 30

[Masatoshi Kimura [:emk]]

Attachment: rebased to tip

Comment 31

[Brian R. Bondy [:bbondy]]

that was fast, thanks :)

Comment 32

[Jim Mathies [:jimm]]

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

Comment 33

[Jim Mathies [:jimm]]

Resubmitted and filed bug 729199 on running individual Talos tests.

Comment 34

[Jim Mathies [:jimm]]

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

Comment 35

[Masatoshi Kimura [:emk]]

> 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

Comment 36

[Jim Mathies [:jimm]]

(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.

Comment 37

[Jim Mathies [:jimm]]

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

Comment 38

[Jim Mathies [:jimm]]

Attachment: netwerk code

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

Comment 39

[Jim Mathies [:jimm]]

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.

Comment 40

[Jim Mathies [:jimm]]

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

Comment 41

[Jim Mathies [:jimm]]

Attachment: netwerk fix

Comment 42

[Jim Mathies [:jimm]]

Attachment: netwerk fix

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.

Comment 43

[Jim Mathies [:jimm]]

pushed to try:

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

Comment 44

[Jim Mathies [:jimm]]

(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.

Comment 45

[Masatoshi Kimura [:emk]]

Attachment: rebased to latest mozilla-inbound

Comment 46

[Jim Mathies [:jimm]]

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

\o/

Comment 48

[Richard Newman [:rnewman]]

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

Comment 49

[Eric Shepherd [:sheppy]]

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