Closed Bug 412374 Opened 12 years ago Closed 12 years ago

Download Mgr bypasses Vista Parental Controls' file-blocking flag

Categories

(Firefox :: Shell Integration, defect, P2, major)

x86
Windows Vista
defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: stephend, Assigned: jimm)

References

(Blocks 3 open bugs, )

Details

Attachments

(1 file, 10 obsolete files)

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
Flags: blocking-firefox3?
Assignee: nobody → jmathies
in-litmus+

https://litmus.mozilla.org/show_test.cgi?id=5119
Flags: in-litmus+
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
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.)
Blocks: 355555
Blocks: 372905
Attached patch pcsvc patch v.1 (obsolete) — Splinter Review
Attached patch pcsvc new files v.1 (obsolete) — Splinter Review
Attachment #302637 - Flags: review?(sdwilsh)
Attachment #302638 - Flags: review?(robert.bugzilla)
Just a note, some of this was based on some work Doug did in bug 355555.
Unified diff, please? Can you put this together into one patch, too? I don't see any reason for it to be separate.
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.
Attached patch pcsvc full patch v.1 (obsolete) — Splinter Review
>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.

Attachment #302638 - Attachment is obsolete: true
Attachment #302638 - Flags: review?(robert.bugzilla)
Attachment #302637 - Attachment is obsolete: true
Attachment #302637 - Flags: review?(sdwilsh)
Attachment #302649 - Flags: review?(robert.bugzilla)
Attachment #302649 - Flags: review?(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 <dolske@mozilla.com> (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.
Attachment #302649 - Flags: review?(sdwilsh) → review-
> >+  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.
(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.
Attached patch pcsvc full patch v.2 (obsolete) — Splinter Review
Attachment #302649 - Attachment is obsolete: true
Attachment #302649 - Flags: review?(robert.bugzilla)
Attached patch pcsvc full patch v.2 (obsolete) — Splinter Review
- removed some commented out code
Attachment #305587 - Attachment is obsolete: true
Attachment #305591 - Flags: superreview?(benjamin)
Attachment #305591 - Flags: review?(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
Attachment #305591 - Flags: review?(sdwilsh) → review+
> >+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
(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!
> 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. 
(In reply to comment #18)
> I did. :)
Sorry - that's what I get for not looking at the new patch before commenting.
Attached patch pcsvc full patch v.3 (obsolete) — Splinter Review
various updates
Attachment #305591 - Attachment is obsolete: true
Attachment #305591 - Flags: superreview?(benjamin)
Comment on attachment 305782 [details] [diff] [review]
pcsvc full patch v.3
Attachment #305782 - Flags: superreview?(benjamin)
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)
Attachment #305782 - Flags: superreview?(benjamin) → superreview-
Whiteboard: [needs new patch]
Attached patch pcsvc full patch v.4 (obsolete) — Splinter Review
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.
Attachment #305782 - Attachment is obsolete: true
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.
Attachment #308979 - Attachment is obsolete: true
Attached patch pcsvc full patch v.5 (obsolete) — Splinter Review
Attachment #310020 - Flags: superreview?(benjamin)
Whiteboard: [needs new patch]
Blocks: 353098
Blocks: 423587
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
Attachment #310020 - Flags: superreview?(benjamin) → superreview+
Attached patch pcsvc full patch v.6 (obsolete) — Splinter Review
Attachment #310020 - Attachment is obsolete: true
Keywords: checkin-needed
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
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 beta5
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Firefox 3 beta5 → Firefox 3
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.
Depends on: 424159
Not sure if it was just coincidence, but the one Windows Thunderbird build while this was in died in the -CreateProfile part of MozillaAliveTest.
In addition, I need to modify this slightly to use getprocaddress on the event calls which are new to the platform.
Attached patch pcsvc full patch v.7 (obsolete) — Splinter Review
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.
Attachment #310552 - Attachment is obsolete: true
Summary: Parental Controls can be easily bypassed by going back to the download source → Download Mgr bypasses Vista Parental Controls file blocking flag
Attachment #310815 - Flags: approval1.9b5?
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.
Is there any way to get tests for this?
(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
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.
Summary: Download Mgr bypasses Vista Parental Controls file blocking flag → Download Mgr bypasses Vista Parental Controls' file-blocking 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 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.
Attachment #310815 - Flags: approval1.9b5?
Attachment #310815 - Flags: approval1.9b5-
Attachment #310815 - Flags: approval1.9?
Attachment #310815 - Flags: approval1.9?
Whiteboard: [has patch][has reviews]
Attachment #310815 - Attachment is obsolete: true
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
Status: REOPENED → RESOLVED
Closed: 12 years ago12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has reviews]
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
I suspect it's an out of date SDK?
Is the Vista SDK now required for building on Windows? I have VC8SP1, and last time I checked, that was a supported SDK...
BTW, this checkin broke the Sunbird tinderboxen as well.
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?

Sunbird and Lightning tinderbox are busted with the same error mentioned in Comment #42: <http://tinderbox.mozilla.org/Sunbird/>
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.

(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

Right, but it should be a part of the Vista SDK.  We do runtime checks that determine if this api is available.
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
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.
Depends on: 425974
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.
(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.
(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.
> 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..
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 ?????
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
>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.
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.
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.
Depends on: 425979
>> 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.

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?
Depends on: 426065
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...
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.) 


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



(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).
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.)
Status: RESOLVED → VERIFIED
Blocks: 430730
You need to log in before you can comment on or make changes to this bug.