Last Comment Bug 699247 - Remove support for executing on Windows 2000
: Remove support for executing on Windows 2000
Status: RESOLVED FIXED
[dev-doc: see comment 39, 42]
: dev-doc-complete
Product: Core
Classification: Components
Component: General (show other bugs)
: Trunk
: All Windows 7
: -- normal with 1 vote (vote)
: mozilla13
Assigned To: Masatoshi Kimura [:emk]
:
:
Mentors:
https://developer.mozilla.org/en/XPCO...
: 723816 (view as bug list)
Depends on: WinNt4Removal msvc2010 RequireWin7SDK 730212 730550 736435
Blocks: 373266 726144 488847 558559 719983 730211 731704
  Show dependency treegraph
 
Reported: 2011-11-02 15:14 PDT by Matheus Kerschbaum
Modified: 2012-12-04 08:39 PST (History)
13 users (show)
matjk7: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (141.61 KB, patch)
2012-02-07 02:58 PST, Masatoshi Kimura [:emk]
jmathies: review+
Details | Diff | Splinter Review
patch v2 (140.74 KB, patch)
2012-02-07 05:49 PST, Masatoshi Kimura [:emk]
neil: superreview+
jacek: feedback+
Details | Diff | Splinter Review
Remove more version checks (5.32 KB, patch)
2012-02-08 03:42 PST, Masatoshi Kimura [:emk]
jmathies: review+
neil: superreview+
Details | Diff | Splinter Review
patch for check in; r=jmathies, sr=neil (145.49 KB, patch)
2012-02-08 05:20 PST, Masatoshi Kimura [:emk]
VYV03354: review+
VYV03354: superreview+
Details | Diff | Splinter Review
bug 719983 fixes (5.50 KB, patch)
2012-02-15 11:20 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
bug 719983 fixes (7.08 KB, patch)
2012-02-15 11:22 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
rebased to tip (140.22 KB, text/plain)
2012-02-17 17:25 PST, Masatoshi Kimura [:emk]
no flags Details
rebased to tip (142.35 KB, patch)
2012-02-17 17:46 PST, Masatoshi Kimura [:emk]
jmathies: review-
Details | Diff | Splinter Review
rebased to tip (142.73 KB, patch)
2012-02-19 08:15 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
netwerk code (17.05 KB, patch)
2012-02-22 06:17 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
netwerk fix (1.06 KB, patch)
2012-02-22 09:08 PST, Jim Mathies [:jimm]
no flags Details | Diff | Splinter Review
netwerk fix (2.01 KB, patch)
2012-02-22 09:32 PST, Jim Mathies [:jimm]
m_kato: review+
Details | Diff | Splinter Review
rebased to latest mozilla-inbound (142.97 KB, patch)
2012-02-23 05:15 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review

Description Matheus Kerschbaum 2011-11-02 15:14:21 PDT
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] 2012-02-07 02:58:43 PST
Created attachment 594969 [details] [diff] [review]
patch

Jacek, could you verify that this patch will not break MinGW builds badly?
Comment 2 Jim Mathies [:jimm] 2012-02-07 05:11:17 PST
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] 2012-02-07 05:49:57 PST
Created attachment 595004 [details] [diff] [review]
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 2012-02-07 06:25:26 PST
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] 2012-02-08 03:42:48 PST
Created attachment 595370 [details] [diff] [review]
Remove more version checks
Comment 6 neil@parkwaycc.co.uk 2012-02-08 04:55:18 PST
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] 2012-02-08 05:20:17 PST
Created attachment 595389 [details] [diff] [review]
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] 2012-02-08 05:29:39 PST
(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] 2012-02-08 07:37:05 PST
https://tbpl.mozilla.org/?tree=Try&rev=e29ff3429d08
Comment 11 Jim Mathies [:jimm] 2012-02-08 13:01:59 PST
(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] 2012-02-08 13:07:31 PST
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] 2012-02-08 13:11:22 PST
Two sets pushed to try config'd to report to each bug.
Comment 14 Jim Mathies [:jimm] 2012-02-08 16:07:42 PST
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] 2012-02-08 16:53:25 PST
(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 2012-02-08 17:00:19 PST
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
Comment 17 Masatoshi Kimura [:emk] 2012-02-08 17:19:34 PST
(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] 2012-02-08 17:26:16 PST
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] 2012-02-11 18:07:17 PST
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] 2012-02-15 10:11:34 PST
(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] 2012-02-15 11:20:05 PST
Created attachment 597495 [details] [diff] [review]
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] 2012-02-15 11:22:46 PST
Created attachment 597497 [details] [diff] [review]
bug 719983 fixes

updates.
Comment 23 Masatoshi Kimura [:emk] 2012-02-15 19:31:20 PST
(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] 2012-02-16 04:49:59 PST
(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] 2012-02-17 17:02:13 PST
(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] 2012-02-17 17:25:49 PST
Created attachment 598454 [details]
rebased to tip
Comment 27 Masatoshi Kimura [:emk] 2012-02-17 17:46:34 PST
Created attachment 598463 [details] [diff] [review]
rebased to tip
Comment 28 Jim Mathies [:jimm] 2012-02-17 18:01:23 PST
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] 2012-02-19 08:14:26 PST
jimm could you rebase to tip? GfxInfo.cpp changes are not applying:
Comment 30 Masatoshi Kimura [:emk] 2012-02-19 08:15:30 PST
Created attachment 598664 [details] [diff] [review]
rebased to tip
Comment 31 Brian R. Bondy [:bbondy] 2012-02-19 08:16:09 PST
that was fast, thanks :)
Comment 33 Jim Mathies [:jimm] 2012-02-21 11:00:40 PST
Resubmitted and filed bug 729199 on running individual Talos tests.
Comment 34 Jim Mathies [:jimm] 2012-02-21 15:48:40 PST
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] 2012-02-21 19:39:20 PST
> 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] 2012-02-21 21:06:13 PST
(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 38 Jim Mathies [:jimm] 2012-02-22 06:17:20 PST
Created attachment 599587 [details] [diff] [review]
netwerk code

Ok, thanks for catching that Masatoshi. netwerk it is!
Comment 39 Jim Mathies [:jimm] 2012-02-22 08:52:35 PST
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] 2012-02-22 08:57:37 PST
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] 2012-02-22 09:08:24 PST
Created attachment 599640 [details] [diff] [review]
netwerk fix
Comment 42 Jim Mathies [:jimm] 2012-02-22 09:32:12 PST
Created attachment 599655 [details] [diff] [review]
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] 2012-02-22 09:43:25 PST
pushed to try:

https://tbpl.mozilla.org/?noignore=0&tree=Try&rev=92f91f90a91c
Comment 44 Jim Mathies [:jimm] 2012-02-22 13:45:29 PST
(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] 2012-02-23 05:15:39 PST
Created attachment 599957 [details] [diff] [review]
rebased to latest mozilla-inbound
Comment 47 Masatoshi Kimura [:emk] 2012-02-23 15:15:32 PST
*** Bug 723816 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.