412374 - Download Mgr bypasses Vista Parental Controls' file-blocking flag

Status: VERIFIED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

[Stephen Donner [:stephend] Not actively reading bugmail]

Summary: Parental Controls can be easily bypassed by going back to the download source

Build ID: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3pre) Gecko/2008011405 Minefield/3.0b3pre

Steps to Reproduce:

1. Enable Parental Controls in Vista Home Premium or Vista Ultimate
2. Block downloads
3. Http://www.mozilla.com/en-US/products/download.html?product=firefox-2.0.0.11&os=win&lang=en-US and download Firefox
4. If PC are working, you'll get blocked
5. Click the browser's Back button and download again

Expected Results:

Download continues to be blocked

Actual Results:

Download is (now) permitted, strangely

Comment 1

[Stephen Donner [:stephend] Not actively reading bugmail]

in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5119

Comment 2

[Jim Mathies [:jimm]]

In testing I was able to get around this completely, the "block file downloads" option is handled up at the browser level. To add support, we'll need to hook into the IWPCWebSettings COM interface to query whether file downloads are blocked, and then act accordingly in the download manager. (note MSDN really doesn't have much on this, see the header file wpcapi.h in the latest sdk for detail.)

Comment 3

[Jim Mathies [:jimm]]

Attachment: pcsvc patch v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: pcsvc new files v.1

Comment 5

[Jim Mathies [:jimm]]

Just a note, some of this was based on some work Doug did in bug 355555.

Comment 6

[Reed Loden [:reed]]

Unified diff, please? Can you put this together into one patch, too? I don't see any reason for it to be separate.

Comment 7

[Doug Turner (:dougt)]

just the good stuff was based on my patches. :-)  

I do not recall ever hooking up the download stuff to all of the consumers.  I remember that there were a bunch of difference places that we needed to check if downloads were allowed and that there were a few edge cases that were hard or impossible to handle.

Comment 8

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.1

Comment 9

[Jim Mathies [:jimm]]

>I
>remember that there were a bunch of difference places that we needed to check
>if downloads were allowed and that there were a few edge cases that were hard
>or impossible to handle.

Specific to all downloads or just upper level dm manager file downloads? Note vista parental controls will block anything web related. This fix relates file downloads that would otherwise pass through the web content filter. It's an upper level switch implemented at the application level.

The idea here was to implement the service everyone was suggesting in bug 355555, used in querying settings and for logging application level pc events. Handling blocked downloads that are contained in pages we display is a different matter, but could leverage this service for things like override requests.

Comment 10

[Shawn Wilsher :sdwilsh]

Comment on attachment 302649 [details] [diff] [review]
pcsvc full patch v.1

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>+#if defined(XP_WIN)
>+  // Check with parental controls to see if file downloads
>+  // are allowed for this user. If not allowed, cancel the
>+  // download and mark its state as being blocked.
>+  nsCOMPtr<nsIParentalControlsService> pc =
>+    do_CreateInstance("@mozilla.org/parental-controls-service;1");
use the #define here please

>+  if (pc) {
>+    PRBool enabled = PR_FALSE;
>+    (void)pc->GetBlockFileDownloadsEnabled(&enabled);
>+    if (enabled) {
>+      (void)CancelDownload(id);
>+      (void)dl->SetState(nsIDownloadManager::DOWNLOAD_BLOCKED);
>+    }
>+    // Log the event if required by pc settings. 
this comment is misleading - it looks like we always log

>Index: toolkit/components/parentalcontrols/Makefile.in
>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 1998
>+# the Initial Developer. All Rights Reserved.
really??

>Index: toolkit/components/parentalcontrols/public/Makefile.in
>+# The Initial Developer of the Original Code is Mozilla Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 2007
>+# the Initial Developer. All Rights Reserved.
>+#
>+# Contributor(s):
>+#  Justin Dolske <[e-mail]> (original author)
?

>+  /**
>+   * Request that blocked URI(s) be allowed through parental
>+   * control filters. Returns true if the URI was successfully
>+   * overriden. Note, may block while native UI is shown.
>+   *
>+   * @param aTarget(s)          URI to be overridden. In the case of
>+   *                            multiple URI, the first URI in the array
>+   *                            should be the root URI of the site.
>+   * @param window              Window that generats the event.
>+   */
>+  boolean requestURIOverride(in nsIURI aTarget, [optional] in nsIWidget window);
>+  boolean requestURIOverrides(in nsIArray aTargets, [optional] in nsIWidget window);
generally I've seen the use of an nsIInterfaceRequestor to get windows.  You may want to check on that.

>+
>+  /**
>+   * @returns true if the current user account has parental controls
>+   * logging enabled. If true, applications should log relevent events
>+   * using 'log'.
>+   */ 
>+  readonly attribute boolean loggingEnabled;
shouldn't the DM code check this before we log?

>+%{C++
>+#define NS_PARENTALCONTROLSSERVICE_CONTRACTID "@mozilla.org/parental-controls-service;1"
>+#define NS_PARENTALCONTROLSSERVICE_CLASSNAME "Parental Controls Service"
>+// {EDB0490E-1DD1-11B2-83B8-DBF8D85906A6}
>+#define NS_PARENTALCONTROLSSERVICE_CID \
>+   { 0x580530e5, 0x118c, 0x4bc7, { 0xab, 0x88, 0xbc, 0x2c, 0xd2, 0xb9, 0x72, 0x23 } }
>+%}
are we supposed to be doing this anymore in the idl?  Also, you already defined the CONTRACTID in toolkit/components/build/nsToolkitCompsCID.h.

Comment 11

[Jim Mathies [:jimm]]

> >+  nsCOMPtr<nsIParentalControlsService> pc =
> >+    do_CreateInstance("@mozilla.org/parental-controls-service;1");
>use the #define here please

updated.

> >+    // Log the event if required by pc settings. 
> this comment is misleading - it looks like we always log
> >+  readonly attribute boolean loggingEnabled;
> shouldn't the DM code check this before we log?

Not really needed since the log call checks it internally. I added the flag property for situations where we might be doing a lot of networking and wanted to cache the value. I've updated the dm code though to get the flag and check it's value for clarity.

> >+# The Initial Developer of the Original Code is
> >+# Netscape Communications Corporation.
> >+# Portions created by the Initial Developer are Copyright (C) 1998
> >+# the Initial Developer. All Rights Reserved.
> really??

Yes since every one of these started out as a file already in the source tree that was copied and modified to work with the pc stuff. If there's something more appropriate to put here please let me know.

> >+   { 0x580530e5, 0x118c, 0x4bc7, { 0xab, 0x88, 0xbc, 0x2c, 0xd2, 0xb9, 0x72, 0x23 } }
> >+%}
> are we supposed to be doing this anymore in the idl?

> Also, you already defined
> > the CONTRACTID in toolkit/components/build/nsToolkitCompsCID.h.

Well, various other idls do this, including the download manager idl and some of the idls I've worked with in exthandler. Looks like the contract id and svc description aren't needed though so I've removed them.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Well, various other idls do this, including the download manager idl and some
> of the idls I've worked with in exthandler. Looks like the contract id and svc
> description aren't needed though so I've removed them.
Just because it's like that elsewhere in the codebase doesn't mean we are supposed to be doing it for new interfaces.  Please check with someone like bsmedberg on this.

Comment 13

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.2

Comment 14

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.2

- removed some commented out code

Comment 15

[Shawn Wilsher :sdwilsh]

Comment on attachment 305591 [details] [diff] [review]
pcsvc full patch v.2

>Index: toolkit/components/downloads/src/nsDownloadManager.cpp
>+#define PARENTALCTRLS_CONTRACTID "@mozilla.org/parental-controls-service;1"
we should be able to use the define in nsToolkitCompsCID.h for this - it shouldn't need to be defined here.

>+nsParentalControlsService::nsParentalControlsService()
>+{
>+}
>+
>+nsParentalControlsService::~nsParentalControlsService()
>+{
>+}
You should be able to use the default constructor and destructor here

>+NS_IMETHODIMP
>+nsParentalControlsService::GetParentalControlsEnabled(PRBool *aResult)
>+NS_IMETHODIMP
>+nsParentalControlsService::GetBlockFileDownloadsEnabled(PRBool *aResult)
>+NS_IMETHODIMP
>+nsParentalControlsService::GetLoggingEnabled(PRBool *aResult)
shouldn't these all return NS_ERROR_NOT_IMPLEMENTED?

>Index: toolkit/components/parentalcontrols/src/nsParentalControlsService.h
>+#include "nsIParentalControlsService.h"
>+#include "nsCOMPtr.h"
>+#include "nsIURI.h"
>+#include "nsIFile.h"
>+#include "nsIWidget.h"
not sure all (if any) of these are needed.  It can help with build times!

>+class nsParentalControlsService : public nsIParentalControlsService
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSIPARENTALCONTROLSSERVICE
>+
>+  nsParentalControlsService();
>+  virtual ~nsParentalControlsService();
again - defaults should be OK here?

>+nsParentalControlsServiceWin::nsParentalControlsServiceWin() : nsParentalControlsService(),
>+mPC(nsnull),
>+mEnabled(PR_FALSE),
>+mProvider(nsnull)
I think it's generally better to do this like this to preserve blame:
nsParentalControlsServiceWin::nsParentalControlsServiceWin() :
    mPC(nsnull)
  , mEnabled(PR_FALSE)
  , mProvider(nsnull)

>+NS_IMETHODIMP
>+nsParentalControlsServiceWin::Log(PRInt16 aEntryType, PRBool blocked, nsIURI *aSource, nsIFile *aTarget)
>+{
>+  if (!mEnabled)
>+    return NS_ERROR_NOT_AVAILABLE;
>+
>+  if (!aSource)
>+    return NS_ERROR_INVALID_ARG;
nit: use NS_ENSURE_ARG_PTR for both aSource and aTarget

>+// Override a single URI
>+NS_IMETHODIMP
>+nsParentalControlsServiceWin::RequestURIOverride(nsIURI *aTarget, nsIInterfaceRequestor *aWindowContext, PRBool *_retval)
ditto for aTarget and aWindowContext

>+  nsCAutoString spec;
>+  aTarget->GetSpec(spec);
check return value please

>+nsParentalControlsServiceWin::RequestURIOverrides(nsIArray *aTargets, nsIInterfaceRequestor *aWindowContext, PRBool *_retval)
ditto for aTargets and aWindowContext

>+  if (arrayLength == 1) {
>+    nsCOMPtr<nsIURI> uri;
>+    aTargets->QueryElementAt(0, NS_GET_IID(nsIURI), (void**)&uri);
this could fail - you should probably check it

>+  // The first entry should be the root uri
>+  nsCAutoString rootSpec;
>+  nsCOMPtr<nsIURI> rootURI;
>+  aTargets->QueryElementAt(0, NS_GET_IID(nsIURI), (void**)&rootURI);
ditto here

>+  if (!rootURI)
>+    return NS_ERROR_INVALID_ARG;
>+  rootURI->GetSpec(rootSpec);
and declare rootSpec closer to where you use it please

>+  for (idx = 1; idx < arrayLength; idx++)
>+  {
>+    nsCOMPtr<nsIURI> uri;
>+    aTargets->QueryElementAt(idx, NS_GET_IID(nsIURI), (void**)&uri);
>+    if (!uri)
>+      continue;
>+
>+    nsCAutoString subURI;
>+    uri->GetSpec(subURI);
should check this, but continue on failure so we don't leak

>+  // Free up the memory
>+  for (idx = 0; idx < count; idx++)
>+    NS_Free((void*)urls[idx]);
docs say that things allocated with UTF8ToNewUnicode should be free'd with nsMemory::Free, not NS_Free.

r=sdwilsh with these changes

Comment 16

[Jim Mathies [:jimm]]

> >+NS_IMETHODIMP
> >+nsParentalControlsService::GetParentalControlsEnabled(PRBool *aResult)
> >+NS_IMETHODIMP
> >+nsParentalControlsService::GetBlockFileDownloadsEnabled(PRBool *aResult)
> >+NS_IMETHODIMP
> >+nsParentalControlsService::GetLoggingEnabled(PRBool *aResult)
> shouldn't these all return NS_ERROR_NOT_IMPLEMENTED?

Hmm, NS_ERROR_NOT_IMPLEMENTED or NS_OK - it makes little difference. I do think aResult should be populated with a default though. Updated.

> >+  if (!mEnabled)
> >+    return NS_ERROR_NOT_AVAILABLE;
> >+
> >+  if (!aSource)
> >+    return NS_ERROR_INVALID_ARG;
> nit: use NS_ENSURE_ARG_PTR for both aSource and aTarget
...
> >+// Override a single URI
> >+NS_IMETHODIMP
> >+nsParentalControlsServiceWin::RequestURIOverride(nsIURI *aTarget,
> nsIInterfaceRequestor *aWindowContext, PRBool *_retval)
> ditto for aTarget and aWindowContext

Updated mostly, though note, aTarget and aWindowContext are optional. aTarget because the file may have been blocked, so there's no local path. aWindowContext is a little different, and I debated making it a requirement but decided not to since this might be called from non ui related code.

> >+  if (!rootURI)
> >+    return NS_ERROR_INVALID_ARG;
> >+  rootURI->GetSpec(rootSpec);
> and declare rootSpec closer to where you use it please

I'd rather do it prior to the allocations, especially now since I've added a result check on it.

> >+  // Free up the memory
> >+  for (idx = 0; idx < count; idx++)
> >+    NS_Free((void*)urls[idx]);
> docs say that things allocated with UTF8ToNewUnicode should be free'd with
> nsMemory::Free, not NS_Free.

I believe nsMemory::Free just calls NS_Free. (what docs were you looking at, I'm just looking at the code.) 

This raises a related question - should I use 'malloc' or 'NS_Alloc' or 'Alloc' to begin with. I think the NS_XXX macros are the most appropriate but I'll check.
http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsMemory.h#64

Comment 17

[Shawn Wilsher :sdwilsh]

(In reply to comment #16)
> Updated mostly, though note, aTarget and aWindowContext are optional. aTarget
> because the file may have been blocked, so there's no local path.
> aWindowContext is a little different, and I debated making it a requirement but
> decided not to since this might be called from non ui related code.
Then you should use the [optional] flag in the idl.  (note, I don't see any docs on it, but there are lots of examples in the code base).

> I believe nsMemory::Free just calls NS_Free. (what docs were you looking at,
> I'm just looking at the code.) 
I'm looking at the header file here:
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/xpcom/string/public/nsReadableUtils.h&rev=1.52#157

> This raises a related question - should I use 'malloc' or 'NS_Alloc' or 'Alloc'
> to begin with. I think the NS_XXX macros are the most appropriate but I'll
> check.
> http://lxr.mozilla.org/mozilla/source/xpcom/glue/nsMemory.h#64
That I cannot answer - bsmedberg can though!

Comment 18

[Jim Mathies [:jimm]]

> Then you should use the [optional] flag in the idl.  (note, I don't see any
> docs on it, but there are lots of examples in the code base).

boolean requestURIOverride(in nsIURI aTarget, [optional] in nsIInterfaceRequestor aWindowContext); 
boolean requestURIOverrides(in nsIArray aTargets, [optional] in nsIInterfaceRequestor aWindowContext); 
void log(in short aEntryType, in boolean aFlag, in nsIURI aSource, [optional] in nsIFile aTarget);

I did. :)

> > >+  // Free up the memory
> > >+  for (idx = 0; idx < count; idx++)
> > >+    NS_Free((void*)urls[idx]);
> > docs say that things allocated with UTF8ToNewUnicode should be free'd with
> > nsMemory::Free, not NS_Free.
> > That I cannot answer - bsmedberg can though!

Found this -
 
http://developer.mozilla.org/en/docs/Choosing_the_right_memory_allocator

I agree with you though since UTF8ToNewUnicode uses nsMemory that seems appropriate there.  I'll use NS_Alloc/NS_Free for the array. 

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> I did. :)
Sorry - that's what I get for not looking at the new patch before commenting.

Comment 20

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.3

various updates

Comment 21

[Jim Mathies [:jimm]]

Comment on attachment 305782 [details] [diff] [review]
pcsvc full patch v.3

Comment 22

[Benjamin Smedberg]

Comment on attachment 305782 [details] [diff] [review]
pcsvc full patch v.3

Overall comments: I think we should just build the parental-controls implementation on Windows. I don't see any reason we should build a "null" PC impl on other platforms. If p-c client code can't create the p-c service/component, it should proceed as if PC were not enabled. This we should build parental-controls/public all the time (so that the interface is always available) but p-c/src only on windows.

This will also allow extensions to implement an alternate p-c implementation on non-Windows, if they wish.


>Index: toolkit/components/downloads/src/nsDownloadManager.cpp

>+#if defined(XP_WIN)
>+  // Check with parental controls to see if file downloads
>+  // are allowed for this user. If not allowed, cancel the
>+  // download and mark its state as being blocked.

Please remove the XP_WIN ifdef around this block, so that extensions may hook the same functionality.

>Index: toolkit/components/parentalcontrols/public/Makefile.in

>+# The Initial Developer of the Original Code is
>+# Netscape Communications Corporation.
>+# Portions created by the Initial Developer are Copyright (C) 1998
>+# the Initial Developer. All Rights Reserved.

Here and in other places, these are incorrect. The initial developer of new code by MoCo employees is the Mozilla Foundation, copyright 2008.

>+%{C++
>+// {EDB0490E-1DD1-11B2-83B8-DBF8D85906A6}
>+#define NS_PARENTALCONTROLSSERVICE_CID \
>+   { 0x580530e5, 0x118c, 0x4bc7, { 0xab, 0x88, 0xbc, 0x2c, 0xd2, 0xb9, 0x72, 0x23 } }
>+%}

This block should be in nsParentalControlsServiceWin.h

>Index: toolkit/components/parentalcontrols/src/Makefile.in

>+#ifeq ($(MOZ_WIDGET_TOOLKIT),windows)
>+#OSDIR		= win
>+#endif

Unnecessary commented block, please remove.

>+REQUIRES  = xpcom \
>+            string \
...

style nit: make this

REQUIRES = \
  xpcom \
  string \
  ... \
  $(NULL)

with no tabs and two-space indent

>+EXTRA_DSO_LDOPTS += $(MOZ_COMPONENT_LIBS)

This is always a static library, this line is unused, please remove it.

>Index: toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp

It is safe and recommended to use CComPtr in Windows code. Please do that for

nsParentalControlsServiceWin::mPC

nsParentalControlsService constructor - wpcs
nsParentalControlsServiceWin::GetLoggingEnabled - wpcs
nsParentalControlsServiceWin::RequestURIOverride - wpcs -- it looks like you leak this one currently!

>+nsParentalControlsServiceWin::RequestURIOverrides(nsIArray *aTargets, nsIInterfaceRequestor *aWindowContext, PRBool *_retval)

>+  // Allocate an array of sub uri
>+  PRInt32 count = arrayLength - 1;
>+  LPCWSTR *urls = (LPCWSTR*) NS_Alloc(count * sizeof(LPCWSTR));
>+  if (!urls)
>+    return NS_ERROR_OUT_OF_MEMORY;

I'd be happier if this were a nsAutoArrayPtr<LPCWSTR*>, but that's just a nit.

>+  PRUint32 uriIdx = 0, idx;
>+  for (idx = 1; idx < arrayLength; idx++)
>+  {
>+    nsCOMPtr<nsIURI> uri;
>+    if (NS_FAILED(aTargets->QueryElementAt(idx, NS_GET_IID(nsIURI), (void**)&uri)))
>+      continue;

Please use do_QueryElementAt from nsArrayUtils.h

>+  IWPCWebSettings * wpcs = nsnull;

CComPtr... you don't currently release this!

>+void
>+nsParentalControlsServiceWin::LogFileDownload(PRBool blocked, nsIURI *aSource, nsIFile *aTarget)
>+{

>+  // Get the exe name of the currently running process

Please use nsIXULAppInfo.name instead of the executable name.

>+  const WCHAR * fill = L"";

static const WCHAR fill[] = L"";

>+
>+  // See wpcevent.h and msdn for event formats
>+  EVENT_DATA_DESCRIPTOR eventData[WPC_ARGS_FILEDOWNLOADEVENT_CARGS];
>+  DWORD dwBlocked = blocked;
>+
>+  EventDataDescCreate(&eventData[WPC_ARGS_FILEDOWNLOADEVENT_URL], (const void*)uri.get(),
>+                      ((ULONG)uri.Length()+1)*sizeof(WCHAR));

I don't think you need the (const void*) cast, and it makes the code less readable IMO.

>+  EventDataDescCreate(&eventData[WPC_ARGS_FILEDOWNLOADEVENT_VERSION], (const void*)fill,
>+                      ((ULONG)wcslen(fill)+1)*sizeof(WCHAR));

You already know the length of "fill", can you just do sizeof(fill) with the changed declaration above? (sizeof L"" should be 2)

Comment 23

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.4

Various updates based on comments. One note, nsCOMPtr didn't like hosting a IWindowsParentalControls as a member variable, so I stuck with what I had. Everything else should be addressed.

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 308979 [details] [diff] [review]
pcsvc full patch v.4

busted make on osx. another patch coming up here in a bit assuming it makes it through the try server this time.

Comment 25

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.5

Comment 26

[Benjamin Smedberg]

Comment on attachment 310020 [details] [diff] [review]
pcsvc full patch v.5

>Index: toolkit/components/parentalcontrols/Makefile.in

>+DIRS = public src

how about

DIRS = public

ifeq (WINNT,$(OS_ARCH))
DIRS += src
endif

then remove the more confusing ifdefs from parentalcontrols/src/Makefile.in

>Index: toolkit/components/parentalcontrols/public/nsIParentalControlsService.idl

>+[scriptable, uuid(871cf229-2b21-4f04-b24d-e08061f14815)]
>+interface nsIParentalControlsService : nsISupports {

nit, put the bracket on the next line

>Index: toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp

>+    nsMemory::Free((void*)arrUrls[idx]);

use NS_Free in new code

Comment 27

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.6

Comment 28

[Reed Loden [:reed]]

Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.78; previous revision: 1.77
done
Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.56; previous revision: 1.55
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.27; previous revision: 1.26
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.51; previous revision: 1.50
done
Checking in toolkit/components/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.20; previous revision: 1.19
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.174; previous revision: 1.173
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/Makefile.in,v
done
Checking in toolkit/components/parentalcontrols/Makefile.in;
/cvsroot/mozilla/toolkit/components/parentalcontrols/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/public/Makefile.in,v
done
Checking in toolkit/components/parentalcontrols/public/Makefile.in;
/cvsroot/mozilla/toolkit/components/parentalcontrols/public/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/public/nsIParentalControlsService.idl,v
done
Checking in toolkit/components/parentalcontrols/public/nsIParentalControlsService.idl;
/cvsroot/mozilla/toolkit/components/parentalcontrols/public/nsIParentalControlsService.idl,v  <--  nsIParentalControlsService.idl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/src/Makefile.in,v
done
Checking in toolkit/components/parentalcontrols/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/parentalcontrols/src/Makefile.in,v  <--  Makefile.in
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp,v
done
Checking in toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp;
/cvsroot/mozilla/toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp,v  <--  nsParentalControlsServiceWin.cpp
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.h,v
done
Checking in toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.h;
/cvsroot/mozilla/toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.h,v  <--  nsParentalControlsServiceWin.h
initial revision: 1.1
done

Comment 29

[Reed Loden [:reed]]

This failed to compile on win2k3 (and probably all non-Vista Windows systems), so I backed out all the changes to files that were already in the tree. I left toolkit/components/parentalcontrols in the tree for now, but I can completely remove it if that's better for you.

c:\slave\trunk_2k3\mozilla\toolkit\components\parentalcontrols\src\nsParentalControlsServiceWin.h(53) : fatal error C1083: Cannot open include file: 'wpcapi.h': No such file or directory

Comment 30

[Jim Mathies [:jimm]]

I ran into similar problems on the try server during testing. It's a tinderbox config problem related to the 6.0 sdk, i'll work it out with build tomorrow.

Comment 31

[Phil Ringnalda (:philor)]

Not sure if it was just coincidence, but the one Windows Thunderbird build while this was in died in the -CreateProfile part of MozillaAliveTest.

Comment 32

[Jim Mathies [:jimm]]

In addition, I need to modify this slightly to use getprocaddress on the event calls which are new to the platform.

Comment 33

[Jim Mathies [:jimm]]

Attachment: pcsvc full patch v.7

Ok, this addresses my bone headed move of elevating WINVER to get at the interfaces without checking platform requirements on the windows event log apis. (They are Vista specific.) The tinderbox sdk maintenance is due to take place on Monday.

Comment 34

[Jim Mathies [:jimm]]

Not sure if this should block or not. It involves some relatively new Vista api, so it should be tested thouroughly. But were so close to B5 now I'm not sure it's a good idea to land it now.

Comment 35

[Mike Beltzner [:beltzner, not reading bugmail]]

Is there any way to get tests for this?

Comment 36

[Shawn Wilsher :sdwilsh]

(In reply to comment #35)
> Is there any way to get tests for this?
not really - it would involve mucking around with system settings somehow

Comment 37

[Jim Mathies [:jimm]]

I'm not sure if this is possible, but it might be nice to have a tests tinderbox image running on an account with features like parental controls enabled. Then we could detect those settings in the unit tests and check for appropriate results.

Comment 38

[John O'Duinn [:joduinn] (please use "needinfo?" flag)]

(In reply to comment #37)
> I'm not sure if this is possible, but it might be nice to have a tests
> tinderbox image running on an account with features like parental controls
> enabled. Then we could detect those settings in the unit tests and check for
> appropriate results.
> 
We can certainly setup new machines to do parental-control-builds *if* that really makes sense. If you feel it does make sense, please file a bug with ReleaseEngineering, covering the following questions to get us going:
- on Vista only or also on other flavours of win32? Does mac have something equivalent?
- do this for "normal" builds or "debug" builds or both?
- how many tests would need to be modified to work in both "normal" and "parental control" modes?
- should failure of these tests close the tree?

Comment 39

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 310815 [details] [diff] [review]
pcsvc full patch v.7

Let's wait until after the beta and throw a bunch of manual tests at it.

Comment 40

[Reed Loden [:reed]]

Attachment: what I will check-in

Comment 41

[Reed Loden [:reed]]

Checking in toolkit/components/Makefile.in;
/cvsroot/mozilla/toolkit/components/Makefile.in,v  <--  Makefile.in
new revision: 1.82; previous revision: 1.81
done
Checking in toolkit/components/build/Makefile.in;
/cvsroot/mozilla/toolkit/components/build/Makefile.in,v  <--  Makefile.in
new revision: 1.58; previous revision: 1.57
done
Checking in toolkit/components/build/nsToolkitCompsCID.h;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsCID.h,v  <--  nsToolkitCompsCID.h
new revision: 1.29; previous revision: 1.28
done
Checking in toolkit/components/build/nsToolkitCompsModule.cpp;
/cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v  <--  nsToolkitCompsModule.cpp
new revision: 1.53; previous revision: 1.52
done
Checking in toolkit/components/downloads/src/Makefile.in;
/cvsroot/mozilla/toolkit/components/downloads/src/Makefile.in,v  <--  Makefile.in
new revision: 1.22; previous revision: 1.21
done
Checking in toolkit/components/downloads/src/nsDownloadManager.cpp;
/cvsroot/mozilla/toolkit/components/downloads/src/nsDownloadManager.cpp,v  <--  nsDownloadManager.cpp
new revision: 1.179; previous revision: 1.178
done
Checking in toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp;
/cvsroot/mozilla/toolkit/components/parentalcontrols/src/nsParentalControlsServiceWin.cpp,v  <--  nsParentalControlsServiceWin.cpp
new revision: 1.2; previous revision: 1.1
done

Comment 42

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Why are my builds on Windows XP now failing with the following error?
d:\mozbuild\mozilla\toolkit\components\parentalcontrols\src\nsParentalControlsServiceWin.h(53) : fatal error C1083: Cannot open include file: 'wpcapi.h': No such file or directory

Comment 43

[Shawn Wilsher :sdwilsh]

I suspect it's an out of date SDK?

Comment 44

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

Is the Vista SDK now required for building on Windows? I have VC8SP1, and last time I checked, that was a supported SDK...

Comment 45

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

BTW, this checkin broke the Sunbird tinderboxen as well.

Comment 46

[Joe Sabash [:JoeS1]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9pre) Gecko/2008032902 Thunderbird/3.0a1pre ID:2008032902
Several nightly testers are reporting the nightly unusable on Vista
http://forums.mozillazine.org/viewtopic.php?p=3317109#3317109

Seems to run fine on Winxp sp2 for me.
Do we need a specific Thunderbird bug?

Comment 47

[Stefan Sitter [:ssitter]]

Sunbird and Lightning tinderbox are busted with the same error mentioned in Comment #42: <http://tinderbox.mozilla.org/Sunbird/>

Comment 48

[Bernd]

What are the build requirements to build this on WinXP, the following configuration does not build:

Visual C++ 8 Express directory: C:\Programme\Microsoft Visual Studio 8\VC\
SDK directory: C:\Programme\Microsoft Platform SDK for Windows Server 2003 R2\
Setting environment for using Microsoft Visual Studio 2005 x86 tools.

cl -?
Microsoft (R) 32-Bit C/C++-Optimierungscompiler Version 14.00.50727.762 fuer 80x86

Before requiring the server 2008

"Another problems seems to occur when you install the Windows SDK for Windows Server 2008. It breaks the vcvars32.bat file from Visual Studio 2008, it no longer sets the correct PATH for the .NET framework and causes an invalid PATH which breaks the Mozilla build. Possible workaround is to use another SDK (the one shipped with Visual Studio 9) or to delete the vcvars32.bat file and repair your Visual Studio installation with the setup program. " 

thats a quote from the windows build requirements.

Comment 49

[Joe Sabash [:JoeS1]]

(In reply to comment #42)
> Why are my builds on Windows XP now failing with the following error?
> d:\mozbuild\mozilla\toolkit\components\parentalcontrols\src\nsParentalControlsServiceWin.h(53)
> : fatal error C1083: Cannot open include file: 'wpcapi.h': No such file or
> directory
> 
Does this help:
http://msdn2.microsoft.com/en-us/library/ms711663(VS.85).aspx

Looks to me that is only available on Vista

Comment 50

[Shawn Wilsher :sdwilsh]

Right, but it should be a part of the Vista SDK.  We do runtime checks that determine if this api is available.

Comment 51

[Jim Mathies [:jimm]]

The apis are only available on Vista. The SDK should have the proper headers.
We had some issues with the try server and one of the test tinderbox due to the
fact they hadn't been upgraded, or their sdk environment paths were messed up.
(bug 424159 was opened on the test box and fixed on Monday.)

Here's the current unit test environment - note the 6.0 sdk paths need to be
first:

http://lxr.mozilla.org/mozilla/source/tools/buildbot-configs/testing/unittest/mozbuild.py#75

Comment 52

[Jim Mathies [:jimm]]

I'm not sure about the thunderbird launch problems from the forums. Sounds like it's related to profiles since people clean their profiles and then everything works. The one crash report provided doesn't point to this patch as the culprit. Not much activity there so I'm not sure but I'll see if I can reproduce.

Comment 53

[Jim Mathies [:jimm]]

Just to clarify - 

> Is the Vista SDK now required for building on Windows? I have VC8SP1, and last
> time I checked, that was a supported SDK...

yes.

Comment 54

[Joe Sabash [:JoeS1]]

(In reply to comment #52)
> I'm not sure about the thunderbird launch problems from the forums. Sounds like
> it's related to profiles since people clean their profiles and then everything
> works. The one crash report provided doesn't point to this patch as the
> culprit. Not much activity there so I'm not sure but I'll see if I can
> reproduce.
> 
I did not have a problem with thunderird, but that might be because I installed
http://www.microsoft.com/downloads/details.aspx?FamilyID=32BC1BEE-A3F9-4C13-9C99-220B62A191EE&displaylang=en
Which may make the API available (not sure)
In my profile @mozilla.org/parental-controls-service;1,{580530e5-118c-4bc7-ab88-bc2cd2b97223} 

Profile cleaning, causes compreg.dat to be re-generated

My point is any testing should be done with an original VC runtime, and not the updated varient.

Comment 55

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

(In reply to comment #53)
> Just to clarify - 
> 
> > Is the Vista SDK now required for building on Windows? I have VC8SP1, and last
> > time I checked, that was a supported SDK...
> 
> yes.
> 

Were you planning on updating the Windows Build Prerequisites accordingly at some point? MDC makes no mention of requiring the Vista SDK currently. In fact, it links to the 2003 Platform SDK.
http://developer.mozilla.org/en/docs/Windows_Build_Prerequisites

Personally, I would think a change of this nature would at least get some kind of advertising, maybe a newsgroup post and/or a blog entry, before just breaking things on people without warning.

Comment 56

[Jim Mathies [:jimm]]

> Were you planning on updating the Windows Build Prerequisites accordingly at
> some point? MDC makes no mention of requiring the Vista SDK currently. In fact,
> it links to the 2003 Platform SDK.

Good point.

> Personally, I would think a change of this nature would at least get some kind
> of advertising, maybe a newsgroup post and/or a blog entry, before just
> breaking things on people without warning.

Not sure if an official announcement was made. From my understanding the migration took place in January. I agree with you though because quite honestly I didn't realize it had happened until I started working on some vista specific stuff in feb..

Comment 57

[Joe Sabash [:JoeS1]]

So, in the original checkin that started this discussion:
Index: toolkit/components/build/nsToolkitCompsModule.cpp
===================================================================
RCS file: /cvsroot/mozilla/toolkit/components/build/nsToolkitCompsModule.cpp,v
retrieving revision 1.52
diff -u -8 -p -r1.52 nsToolkitCompsModule.cpp
--- toolkit/components/build/nsToolkitCompsModule.cpp	20 Mar 2008 06:34:26 -0000	1.52
+++ toolkit/components/build/nsToolkitCompsModule.cpp	29 Mar 2008 07:10:14 -0000
@@ -35,16 +35,21 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
 #include "nsIGenericFactory.h"
 #include "nsAppStartup.h"
 #include "nsUserInfo.h"
 #include "nsXPFEComponentsCID.h"
 #include "nsToolkitCompsCID.h"
+
+#ifdef XP_WIN
+#include "nsParentalControlsServiceWin.h"
+#endif
+

#ifdef XP_WIN means that Vista sdk is installed ?????

Comment 58

[Jim Mathies [:jimm]]

XP_WIN is a platform build define for windows, the 'XP' does not imply "Windows XP". See - 
http://lxr.mozilla.org/mozilla/source/modules/coreincl/platform.h#99

Comment 59

[Jim Mathies [:jimm]]

>In my profile
>@mozilla.org/parental-controls-service;1,{580530e5-118c-4bc7-ab88-bc2cd2b97223} 
>
>Profile cleaning, causes compreg.dat to be re-generated

Hmm, good point. I wonder if this is tied to folks running nightlies who upgrading regularly. I'll have to ask around, I'm not that familiar with the functionality behind the code that manages compreg.dat.

Comment 60

[Phil Ringnalda (:philor)]

Since the bug 425950 order of operations is

1. Install the 2008-03-28 Tbird
2. With it, download the 28->29 partial
3. Click "restart now," crash in GC
4. Restart, find that compreg.dat is hosed

I very strongly doubt that you have anything to do with it - far as I know, at the restart-crash point, your patch is nothing but an entry in the list of things which would have been done later, if things didn't go wrong first. Something in XPCOM/JS/update code that landed before the nightly from the 28th is much more likely.

Comment 61

[Phil Ringnalda (:philor)]

Wups, you're just unlikely, not off the hook - I thought it was a shutdown crash, but after doing it three more times, I see it's actually the post-updater startup that crashes.

Comment 62

[Bernd]

>> Were you planning on updating the Windows Build Prerequisites accordingly at
>> some point? MDC makes no mention of requiring the Vista SDK currently. In fact,
>> it links to the 2003 Platform SDK.

>Good point.

I think if you make the SDK obsolete you should add a configure test that clearly states that the old SDK is not sufficient to build firefox. Having a debug build busted just before checkin in the last phase of compilation is not fun.

Thanks to reed for comment 40 so that I could get operational again by running a local back out.

Comment 63

[:Gijs (he/him)]

Is this supposed to not break VC7.1 on Win XP? If so, what's the magic incantation to make things work after installing the Vista SDK? It doesn't actually add its lib or include directories to the environment, or to vsvars32.bat, and after doing the latter manually, it just complains it can't find headers (sal.h) for those files. Those are shipped in the include dir for the "VC" subdirectory, but that sort of gives me the impression I should be using the link/cl/ml/lib etc. tools that are shipped with that SDK (in aforementioned VC subdirectory) instead of those in VC7.1. Which then leaves me feeling... confused, mostly. Am I missing something completely obvious?

Comment 64

[Jim Mathies [:jimm]]

I'm setting up a 7.1 image here with the latest mozilla-build to see if I can reproduce. We might have to update tools.  This should be working though, I wouldn't think we'd up the sdk requirements without making sure mozilla's build tools were compatible. I'll find out here later today though...

Comment 65

[Jim Mathies [:jimm]]

Changes I had to make to my env for vc 7.1 -

- add bin, include, and lib sdk/VC subdirs
- add an include path to a newer version of the atl headers (a newer sdk or the ones that come with vc8/vcexpress)

After that, the compile succeeded.

Here's what I ended up with that worked:

SET PATH=%SDKDIR%bin;%SDKDIR%VC\bin;%PATH%
SET LIB=%SDKDIR%lib;%SDKDIR%VC\lib;%LIB%
SET INCLUDE=%SDKDIR%include;%SDKDIR%VC\include;C:\Program Files\Microsoft Visual Studio 8\VC\atlmfc\include;%INCLUDE%

(make sure SDKDIR and MOZBUILD_USE_SDK are set as well. Look at the console bat files in mozilla-tools for more info.) 

Comment 66

[Marco Bonardo [:mak]]

for windows server 2008 sdk, i had to fix mozilla-build/guess-msvc.bat with SDK 6.1 values (don't know if there's a new 1.2 build newer than mine with these fixed):

HKLM\SOFTWARE\Microsoft\Microsoft SDKs\Windows\v6.1\WinSDKBuild

Comment 67

[[e-mail]]

(In reply to comment #22)
>(From update of attachment 305782 [details] [diff] [review])
>>+  // Allocate an array of sub uri
>>+  PRInt32 count = arrayLength - 1;
>>+  LPCWSTR *urls = (LPCWSTR*) NS_Alloc(count * sizeof(LPCWSTR));
>>+  if (!urls)
>>+    return NS_ERROR_OUT_OF_MEMORY;
>I'd be happier if this were a nsAutoArrayPtr<LPCWSTR*>, but that's just a nit.
Actually one advantage of the original code would have been the possibility to use NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY to free it.

Comment 68

[:Gijs (he/him)]

(In reply to comment #65)
> Changes I had to make to my env for vc 7.1 -
> 
> - add bin, include, and lib sdk/VC subdirs
> - add an include path to a newer version of the atl headers (a newer sdk or the
> ones that come with vc8/vcexpress)
> 
> After that, the compile succeeded.
> 
> Here's what I ended up with that worked:
> 
> SET PATH=%SDKDIR%bin;%SDKDIR%VC\bin;%PATH%
> SET LIB=%SDKDIR%lib;%SDKDIR%VC\lib;%LIB%
> SET INCLUDE=%SDKDIR%include;%SDKDIR%VC\include;C:\Program Files\Microsoft
> Visual Studio 8\VC\atlmfc\include;%INCLUDE%
> 
> (make sure SDKDIR and MOZBUILD_USE_SDK are set as well. Look at the console bat
> files in mozilla-tools for more info.) 

But I don't have VC8 - if I did, I'd be building with that in the first place, as it's actually supported by the Vista SDK... So if I understand you and the little info I can find on the matter[1,2], I *hope* that these headers are available in the release version of the Windows 2008 server SDK? But even that seems like a long shot, and this PATH/LIB/INCLUDE hacking scares me a bit, too. It'd be much easier if there were --disable flag of some kind...

[1] http://blogs.msdn.com/windowssdk/archive/2007/09/11/sdk-workaround-atl-mfc-sample-dependency.aspx
[2] http://en.wikipedia.org/wiki/Microsoft_Windows_SDK

Comment 69

[(not currently active) Ted Mielczarek]

(In reply to comment #65)
> Changes I had to make to my env for vc 7.1 -

Please, let's not advise people to start hacking their build environments manually like this. This is why we have MozillaBuild in the first place. We should provide a configure switch to disable this feature as suggested in bug 425979 comment 4, and a configure check for the Vista SDK headers so that we will error out in configure if the user doesn't have the SDK (which is bug 426065).

Comment 70

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED for the testcase as originally filed in comment 0, using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008040706 Minefield/3.0pre

(We also honor things like "Web Restrictions," because I searched for "Pacifico Beer" on Google.com and it wouldn't let me load the page with my "Medium" setting which blocked "drugs" among other things; I haven't done _extensive_ testing with this particular feature, but will continue to test and file spinoff bugs if there are any.)