334990 - Patch to netwerk/base/public/nsNetUtil.h needed to be able to compile with Sun Forte 6 Update 2

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[riot.nrrrd.bugzilla]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O) Gecko/20040906 Camino/0.8+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O) Gecko/20040906 Camino/0.8+

While trying to compile Firefox 1.5.0.2 on Solaris 9 with Sun's older Forte 6 Update 2 compiler suite (we cannot upgrade, due to configuration freezes), I hit this problem:

nsInputStreamChannel.cpp
CC -o nsInputStreamChannel.o -c  -DMOZILLA_INTERNAL_API
-DOSTYPE=\"SunOS5\" [...]
nsInputStreamChannel.cpp

"nsInputStreamChannel.cpp", line 395: Error: Could not find a match for
NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor>,
nsCOMPtr<nsILoadGroup>, nsCOMPtr<nsIProgressEventSink>).

1 Error(s) detected.
gmake[5]: *** [nsInputStreamChannel.o] Error 1
gmake[5]: Leaving directory
`/usr/local/src/WWW/mozilla/1.8.0-Firefox/mozilla/netwerk/base/src'

There is another instance (if this instance is fixed) where another file does not compile with Forte 6 Update 2.  A second patch to nsNetUtil.h is required to fix that instance.

Reproducible: Always

Steps to Reproduce:
1. Try to build Firefox 1.5.0.2 from scratch using Sun Forte 6 Update 2.


Actual Results:  
See Details for compilation error.

Expected Results:  
Expected the code to compile cleanly  :-)


I propose this patch, to netwerk/base/public/nsNetUtil.h.  It fixes the problem with both files being unable to be compiled with Sun Forte 6.  Hopefully it will not break other compilers such as GCC on Suns or other OS platforms, or newer versions of Sun's Forte & Workshop compiler suites.

*** netwerk/base/public/nsNetUtil.h.dist        Mon Jul 25 13:27:02 2005
--- netwerk/base/public/nsNetUtil.h     Thu Apr 20 03:14:58 2006
***************
*** 979 ****
--- 980,996 ----
+ /* template helper */
+ template <class T> inline void
+ NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor> aCallbacks,
+                               nsCOMPtr<nsILoadGroup>          aLoadGroup,
+                               nsCOMPtr<T>           &aResult)
+ {
+     NS_QueryNotificationCallbacks(aCallbacks.get(), aLoadGroup.get(), aResult);
+ }
+ 
+ /* template helper */
+ template <class T> inline void
+ NS_QueryNotificationCallbacks(nsCOMPtr<nsIChannel> aChannel,
+                               nsCOMPtr<T>           &aResult)
+ {
+     NS_QueryNotificationCallbacks(aChannel.get(), aResult);
+ }
+

Comment 1

[riot.nrrrd.bugzilla]

Attachment: Proposed patch to netwerk/base/public/nsNetUtil.h

The proposed patch allows Firefox 1.5.0.2 to compile cleanly with Sun Forte 6 Update 2.  Its impact on building with other compilers such as GCC or newer versions of Sun's Forte/Workshop compiler suite is presently unknown.

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

passing non-reference comptrs around is not such a great idea, it causes an additional addref and release...

also, please make patches with -pu6 as diff flags or something

Also... are both versions (with raw pointer and nsCOMPtr) needed, or do all callers have a comptr anyway?

Comment 3

[riot.nrrrd.bugzilla]

Comment on attachment 219342 [details] [diff] [review]
Proposed patch to netwerk/base/public/nsNetUtil.h

--- mozilla/netwerk/base/public/nsNetUtil.h.dist        Mon Jul 25 13:27:02 2005
+++ mozilla/netwerk/base/public/nsNetUtil.h     Thu Apr 20 03:14:58 2006
@@ -974,12 +974,29 @@ NS_QueryNotificationCallbacks(nsIInterfa
 {
     NS_QueryNotificationCallbacks(aCallbacks, aLoadGroup,
                                   NS_GET_IID(T),
                                   getter_AddRefs(aResult));
 }
 
+/* template helper */
+template <class T> inline void
+NS_QueryNotificationCallbacks(nsCOMPtr<nsIInterfaceRequestor> aCallbacks,
+                              nsCOMPtr<nsILoadGroup>          aLoadGroup,
+                              nsCOMPtr<T>           &aResult)
+{
+    NS_QueryNotificationCallbacks(aCallbacks.get(), aLoadGroup.get(), aResult);
+}
+
+/* template helper */
+template <class T> inline void
+NS_QueryNotificationCallbacks(nsCOMPtr<nsIChannel> aChannel,
+                              nsCOMPtr<T>           &aResult)
+{
+    NS_QueryNotificationCallbacks(aChannel.get(), aResult);
+}
+
 /**
  * This function returns a nsIInterfaceRequestor instance that returns the
  * same result as NS_QueryNotificationCallbacks when queried.  It is useful
  * as the value for nsISocketTransport::securityCallbacks.
  */
 inline nsresult

Comment 4

[riot.nrrrd.bugzilla]

(In reply to comment #2)
> passing non-reference comptrs around is not such a great idea, it causes an
> additional addref and release...
> 
> also, please make patches with -pu6 as diff flags or something

Sorry about that ... I updated the attachment (or tried to anyway - my first-ever Mozilla bug report, so I apologize for any blunders).

> Also... are both versions (with raw pointer and nsCOMPtr) needed, or do all
> callers have a comptr anyway?

Christian,

I have no clue ...I don't profess to understand either the code in nsInputStreamChannel.cpp nor the patch, just that it appears that Forte 6 needs some sort of Template helper for the 2 particular cases where it stumbles.  A code guru can undoubtedly make better sense of it than I - I am just passing on the patch I got from Boris Zbarsky of MIT <[e-mail]> over in the mozilla.dev.build newsgroup.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

You can't change attachments once attached. Bugzilla's user interface is a bit confusing there, but your only option is to attach a new version.

cc'ing bz as this seems to be his patch :) see comment 2

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'll buy wanting references; let's do that.  I have no clue on the other; I'd have to audit the code.  It doesn't seem worth trying to do that to me, since all this stuff is inlined anyway....

reporter, want to attach an updated patch that uses "nsCOMPtr<whatever>&" instead of "nsCOMPtr<whatever>"?  At least assuming that still compiles for you?

Comment 7

[riot.nrrrd.bugzilla]

Attachment: New "-pu6" version of proposed patch

"diff -pu6" version attached.

Can you and Boris discuss what the actual patch contents should be, Christian? :)

(I am willing to take an altered patch and do a complete rebuild to test it)

Comment 8

[riot.nrrrd.bugzilla]

(In reply to comment #6)
> reporter, want to attach an updated patch that uses "nsCOMPtr<whatever>&"
> instead of "nsCOMPtr<whatever>"?  At least assuming that still compiles for
> you?

Hi Boris,

If you guys want to submit a modified version, I'm willing to test it out and do a complete rebuild to verify.  I just want it to work; you guys can make it "correct" :)

Greg

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: So does this make Sun Forte work?

Comment 10

[riot.nrrrd.bugzilla]

(In reply to comment #9)
> Created an attachment (id=219395) [edit]
> So does this make Sun Forte work?

Yes, it works - I just finished a complete rebuild from scratch with no compilation problems.

(Now if only it didn't core dump when I click on the "Security" Preference ... but that's a different issue :-) )

Thanks everyone ...

Comment 11

[Darin Fisher]

Comment on attachment 219395 [details] [diff] [review]
So does this make Sun Forte work?

Should the "in" nsCOMPtr's be |const| ?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: Probably a good idea.

Greg, would you mind testing this too?

Comment 13

[Darin Fisher]

Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

r+sr=darin provided it works ;-)  ... please fix indenting of aResult

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

Requesting branch approvals too; this is needed to make branch compile.  I'll merge to trunk and check in by a few hours from now.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed on trunk.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8 branch.

Comment 17

[riot.nrrrd.bugzilla]

(In reply to comment #12)
> Created an attachment (id=219518) [edit]
> Probably a good idea.
> 
> Greg, would you mind testing this too?

Did a full rebuild with the latest patch overnight.  Works fine.  I'd say commit it ...

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 219518 [details] [diff] [review]
Probably a good idea.

approved for 1.8.0 branch, a=dveditz for drivers.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.8.0 branch.  Greg, thanks for the patch!