Last Comment Bug 334990 - Patch to netwerk/base/public/nsNetUtil.h needed to be able to compile with Sun Forte 6 Update 2
: Patch to netwerk/base/public/nsNetUtil.h needed to be able to compile with Su...
Status: RESOLVED FIXED
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: Sun Solaris
: -- normal (vote)
: ---
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
: Patrick McManus [:mcmanus]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-04-21 12:22 PDT by riot.nrrrd.bugzilla
Modified: 2006-04-30 22:54 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed patch to netwerk/base/public/nsNetUtil.h (796 bytes, patch)
2006-04-21 12:26 PDT, riot.nrrrd.bugzilla
no flags Details | Diff | Splinter Review
New "-pu6" version of proposed patch (1.21 KB, patch)
2006-04-21 14:10 PDT, riot.nrrrd.bugzilla
no flags Details | Diff | Splinter Review
So does this make Sun Forte work? (1.72 KB, patch)
2006-04-21 21:27 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details | Diff | Splinter Review
Probably a good idea. (1.74 KB, patch)
2006-04-23 10:44 PDT, Boris Zbarsky [:bz] (still a bit busy)
darin.moz: review+
darin.moz: superreview+
darin.moz: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description riot.nrrrd.bugzilla 2006-04-21 12:22:07 PDT
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 2006-04-21 12:26:34 PDT
Created attachment 219342 [details] [diff] [review]
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) 2006-04-21 13:07:29 PDT
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 2006-04-21 13:32:30 PDT
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 2006-04-21 13:36:01 PDT
(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 <bzbarsky@mit.edu> over in the mozilla.dev.build newsgroup.

Comment 5 Christian :Biesinger (don't email me, ping me on IRC) 2006-04-21 14:02:23 PDT
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 [:bz] (still a bit busy) 2006-04-21 14:10:14 PDT
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 2006-04-21 14:10:54 PDT
Created attachment 219363 [details] [diff] [review]
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 2006-04-21 14:12:35 PDT
(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 [:bz] (still a bit busy) 2006-04-21 21:27:46 PDT
Created attachment 219395 [details] [diff] [review]
So does this make Sun Forte work?
Comment 10 riot.nrrrd.bugzilla 2006-04-22 16:03:40 PDT
(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 2006-04-23 10:22:49 PDT
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 [:bz] (still a bit busy) 2006-04-23 10:44:40 PDT
Created attachment 219518 [details] [diff] [review]
Probably a good idea.

Greg, would you mind testing this too?
Comment 13 Darin Fisher 2006-04-23 14:07:49 PDT
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 [:bz] (still a bit busy) 2006-04-23 16:30:54 PDT
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 [:bz] (still a bit busy) 2006-04-23 19:31:47 PDT
Fixed on trunk.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2006-04-23 20:02:58 PDT
Fixed on 1.8 branch.
Comment 17 riot.nrrrd.bugzilla 2006-04-24 10:29:46 PDT
(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] 2006-04-24 16:45:45 PDT
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 [:bz] (still a bit busy) 2006-04-24 19:21:16 PDT
Fixed on 1.8.0 branch.  Greg, thanks for the patch!

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