Last Comment Bug 694804 - Failure to serialize security info for e10s should be a hard error, not a warning
: Failure to serialize security info for e10s should be a hard error, not a war...
Status: RESOLVED FIXED
[mentor=jdm] [lang=c++]
:
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Mujtaba A
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-15 18:20 PDT by Brian Smith (:briansmith, :bsmith, use NEEDINFO?)
Modified: 2011-10-25 18:06 PDT (History)
5 users (show)
emorley: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Warning changed to error (1.03 KB, patch)
2011-10-22 16:22 PDT, Mujtaba A
michal.novotny: review+
Details | Diff | Splinter Review

Description Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-15 18:20:12 PDT
The code should be something like this:

@@ -204,25 +205,27 @@ WyciwygChannelParent::OnStartRequest(nsI
 
   PRInt32 contentLength = -1;
   chan->GetContentLength(&contentLength);
 
   PRInt32 charsetSource = kCharsetUninitialized;
   nsCAutoString charset;
   chan->GetCharsetAndSource(&charsetSource, charset);
 
-  nsCOMPtr<nsISupports> securityInfo;
+  nsCOMPtr<nsITransportSecurityInfo> securityInfo;
   chan->GetSecurityInfo(getter_AddRefs(securityInfo));
   nsCString secInfoStr;
   if (securityInfo) {
     nsCOMPtr<nsISerializable> serializable = do_QueryInterface(securityInfo);
     if (serializable)
       NS_SerializeToString(serializable, secInfoStr);
-    else
-      NS_WARNING("Can't serialize security info");
+    else {
+      NS_ERROR("Can't serialize security info");
+      return NS_ERROR_UNEXPECTED;
+    }
   }
Comment 2 Mujtaba A 2011-10-19 13:33:19 PDT
As already communicated, I would like to help with this. I will try to submit a patch after I get home later tonight (probably around 1am or so)
Comment 3 Mujtaba A 2011-10-22 16:22:57 PDT
Created attachment 568916 [details] [diff] [review]
Warning changed to error

I did not know how to automatically generate the comment at the top, so, I manually inserted the first two lines. I hope that is okay.
Comment 4 Mujtaba A 2011-10-22 16:24:30 PDT
Hi, 

I did not convert nsISupports to nsITransportSecurityInfo due to the following errors:

/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:212: error: ‘nsITransportSecurityInfo’ was not declared in this scope
/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:212: error: template argument 1 is invalid
/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:212: error: invalid type in declaration before ‘;’ token
/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:213: error: no matching function for call to ‘getter_AddRefs(int&)’
/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:216: error: invalid conversion from ‘int’ to ‘nsISupports*’
/home/mujtaba/firefox/mozilla-central/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:216: error:   initializing argument 1 of ‘nsQueryInterface do_QueryInterface(nsISupports*)’

I have not looked through the code enough to understand how to include nsITransportSecurityInfo works, so, I left it for the proposed patch. I did implement the error though.

Let me know if I need to fix anything!
Comment 5 Josh Matthews [:jdm] 2011-10-22 16:43:41 PDT
You can just include nsITransportSecurityInfo.h to get rid of that error.
Comment 6 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-22 17:46:53 PDT
(In reply to Mujtaba A from comment #4)
> I did not convert nsISupports to nsITransportSecurityInfo due to the
> following errors:

That is OK, because that part of my suggestion was copy/pasted out of a bigger patch that does a bunch of nsISupports -> nsITransportSecurityInfo conversions.
Comment 7 Brian Smith (:briansmith, :bsmith, use NEEDINFO?) 2011-10-22 17:48:16 PDT
(In reply to Josh Matthews [:jdm] from comment #5)
> You can just include nsITransportSecurityInfo.h to get rid of that error.

This won't help, because GetSecurityInfo's parameter requires an nsISupports. nsISupports is specific enough for this patch to work anyway.
Comment 8 Patrick McManus [:mcmanus] 2011-10-23 07:10:48 PDT
Comment on attachment 568916 [details] [diff] [review]
Warning changed to error

looks good to me, but I don't feel comfortable reviewing changes to most wyciwyg or e10s code. Michal or Jason certainly will.
Comment 10 Ed Morley [:emorley] 2011-10-25 18:06:15 PDT
https://hg.mozilla.org/mozilla-central/rev/d6f93b72d804

Hey congrats on your first patch in the tree! 

Hope to see you on IRC (http://irc.mozilla.org/) in #developers soon. If you'd like to fix another bug (we'd love it if you did!) but need some inspiration, pop on & say hi - and we'll find something for you :-)

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