611930 - Crashreporter should provide information on installed LSPs.

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Attachment: Patch

See the URL for an example of a crashreport.  I'm open to suggestions about what data is worth capturing and what format we should print in.

Comment 1

[timeless]

+ *   Kyle Huey <[e-mail]

missing >

but please land this :)

Comment 2

[[e-mail]]

Could we get this for beta8? Freeze is Monday, and I think this would help us understand a big portion of our present crashiness. Which is good to know ASAP. :)

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 490328 [details] [diff] [review]
Patch

>diff --git a/widget/src/windows/LSPAnnotator.cpp b/widget/src/windows/LSPAnnotator.cpp
>new file mode 100644
>--- /dev/null
>+++ b/widget/src/windows/LSPAnnotator.cpp
>+void
>+LSPAnnotationGatherer::Annotate()
>+{
>+  nsCOMPtr<nsICrashReporter> cr =
>+    do_GetService("@mozilla.org/toolkit/crash-reporter;1");
>+  PRBool enabled;
>+  if (cr && NS_SUCCEEDED(cr->GetEnabled(&enabled)) && enabled) {
>+    cr->AnnotateCrashReport(NS_LITERAL_CSTRING("LSPs"), mString);

LSPs doesn't sound descriptive enough. Maybe Winsock_LSP or something?

>+    nsCString note;
>+    note.AppendLiteral("LSPs: ");

nsCString note = NS_LITERAL_CSTRING("LSPs: "); ?

>+    note.Append(mString);
>+    cr->AppendAppNotesToCrashReport(mString);

Please file a bug on having Socorro display the LSP field, and put a comment here with the bug number, so we can remove this from the App Notes bit once that's fixed.

>+  nsCString str;
>+  for (int i = 0; i < n; i++) {
>+    AppendUTF16toUTF8(nsDependentString(providers[i].szProtocol), str);
>+    str.AppendLiteral(" : ");
>+    str.AppendInt(providers[i].iVersion);
>+    str.AppendLiteral(" : ");
>+    str.AppendInt(providers[i].iSocketType);
>+
>+    wchar_t path[MAX_PATH];
>+    int dummy;
>+    if (!WSCGetProviderPath(&providers[i].ProviderId, path,
>+                            &dummy, &err)) {
>+      AppendUTF16toUTF8(nsDependentString(path), str);
>+    }
>+
>+    if (i + 1 != n) {
>+      str.AppendLiteral(" ; ");

I'd stick a newline in here instead to make this more readable.

Comment 4

[Johnny Stenback (:jst)]

Having this in beta8 would be awesome!

Comment 5

[Robert Strong (they/them - no direct email)]

Comment on attachment 490328 [details] [diff] [review]
Patch

Awesome!

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 490328 [details] [diff] [review]
Patch

>diff --git a/widget/src/windows/LSPAnnotator.cpp b/widget/src/windows/LSPAnnotator.cpp
>new file mode 100644
>--- /dev/null
>+++ b/widget/src/windows/LSPAnnotator.cpp
>...
>+NS_IMETHODIMP
>+LSPAnnotationGatherer::Run()
>+{
>+  mThread = NS_GetCurrentThread();
>+
>+  DWORD size = 0;
>+  int err;
>+  if (SOCKET_ERROR != WSCEnumProtocols(NULL, NULL, &size, &err) ||
>+      err != WSAENOBUFS) {
>+    // Er, what?
nit: would be nice if the comment mentioned that this call is just to get size.

>+    NS_NOTREACHED("WSCEnumProtocols should always fail here ...");
"should always fail here" seems wrong to me

>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsAutoPtr<char> byteArray = new char[size];
>+  WSAPROTOCOL_INFO* providers =
>+    reinterpret_cast<WSAPROTOCOL_INFO*>(byteArray.get());
>+
>+  int n = WSCEnumProtocols(NULL, providers, &size, &err);
>+  if (n == SOCKET_ERROR) {
>+    // Lame
nit: better comment as well stating why this should not fail

Might be a good thing to have Jim take a look at this as well

Comment 7

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to comment #6)
> Comment on attachment 490328 [details] [diff] [review]
> Patch
> 
> >diff --git a/widget/src/windows/LSPAnnotator.cpp b/widget/src/windows/LSPAnnotator.cpp
> >new file mode 100644
> >--- /dev/null
> >+++ b/widget/src/windows/LSPAnnotator.cpp
> >...
> >+NS_IMETHODIMP
> >+LSPAnnotationGatherer::Run()
> >+{
> >+  mThread = NS_GetCurrentThread();
> >+
> >+  DWORD size = 0;
> >+  int err;
> >+  if (SOCKET_ERROR != WSCEnumProtocols(NULL, NULL, &size, &err) ||
> >+      err != WSAENOBUFS) {
> >+    // Er, what?
> nit: would be nice if the comment mentioned that this call is just to get size.

OK

> 
> >+    NS_NOTREACHED("WSCEnumProtocols should always fail here ...");
> "should always fail here" seems wrong to me

Passing in a 0 length buffer should always fail (unless, I suppose, the user has 0 LSPs, but Windows ships with built in LSPs).

> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  nsAutoPtr<char> byteArray = new char[size];
> >+  WSAPROTOCOL_INFO* providers =
> >+    reinterpret_cast<WSAPROTOCOL_INFO*>(byteArray.get());
> >+
> >+  int n = WSCEnumProtocols(NULL, providers, &size, &err);
> >+  if (n == SOCKET_ERROR) {
> >+    // Lame
> nit: better comment as well stating why this should not fail
> 
> Might be a good thing to have Jim take a look at this as well

Sure.

Comment 8

[Robert Strong (they/them - no direct email)]

Perhaps then "WSCEnumProtocols succeeded when it should have failed" for clarity

Comment 9

[Robert Strong (they/them - no direct email)]

Comment on attachment 490328 [details] [diff] [review]
Patch

btw: I don't think Jim needs to review this for it to land... I mainly want him to know about this code since he touches windows widget quite often.

Comment 10

[Robert Strong (they/them - no direct email)]

Jim, cc'ing you to give you a heads up about this addition.

Comment 11

[Jim Mathies [:jimm]]

(In reply to comment #10)
> Jim, cc'ing you to give you a heads up about this addition.

Should be interesting to see what shows up.

Comment 12

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/c8a4b4ed9160
and
http://hg.mozilla.org/mozilla-central/rev/4744e56a99b8
to fix some non-IPC unicode issues.

Comment 13

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It looks like this information is getting cut off on the crash reports :-(

Comment 14

[Benjamin Smedberg]

App notes has a limit of 1000 characters or so. This really needs to go in a dedicated field and be stored in the database separately to be much use for aggregate reports, I think.

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Guess we might as well remove this from the App notes then.  We don't get through the MS LSPs on any of the reports :-/

Comment 16

[[e-mail]]

Dunno why, but TestWinDND.cpp is trying to link this and failing because it needs ws2_32.lib (this is a non-libxul-only test which is why trunk is green).

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

It's trying to link statically against gkwidget which uses symbols from this.