The default bug view has changed. See this FAQ.

Failure to serialize security info for e10s should be a hard error, not a warning

RESOLVED FIXED in mozilla10

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: briansmith, Assigned: Mujtaba A)

Tracking

Trunk
mozilla10
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=jdm] [lang=c++])

Attachments

(1 attachment)

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;
+    }
   }

Updated

6 years ago
Whiteboard: [mentor=jdm] [lang=c++]

Comment 1

6 years ago
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp#220
(Assignee)

Comment 2

6 years ago
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)
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
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

6 years ago
You can just include nsITransportSecurityInfo.h to get rid of that error.
(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.
(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.
Attachment #568916 - Flags: review?(mcmanus)
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.
Attachment #568916 - Flags: review?(mcmanus) → review?(michal.novotny)
Attachment #568916 - Flags: review?(michal.novotny) → review+

Comment 9

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/d6f93b72d804

Updated

6 years ago
Assignee: nobody → mujtabaar
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 :-)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.