609844 - Improve usage of QtNetwork

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

Qt Network Implementation is to complicated and has a bug which prevents us from connecting to a network successfully on latest meego.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: patch that updates to latest working implementation

this patch updates to lastest version of qnetwork and internal changes in the way it works. Current upstreamed code is not functional atm.

Comment 2

[Doug Turner (:dougt)]

Comment on attachment 497485 [details] [diff] [review]
patch that updates to latest working implementation

Avoid the static method call.  Replace nsQtNetworkManager::instance() with  gInstance.  Rename it to gQtNetworkManager.

Also, i have no idea what you are trying to build/design.  Could you provide a simple (10-15 sentences) overview of these changes?

I will try to comment on the stuff that looks wrong (or stuff I don't understand very well).



>diff --git a/netwerk/base/src/nsBaseChannel.cpp b/netwerk/base/src/nsBaseChannel.cpp
>--- a/netwerk/base/src/nsBaseChannel.cpp
>+++ b/netwerk/base/src/nsBaseChannel.cpp
>@@ -82,21 +82,21 @@ private:
> #define SUSPEND_PUMP_FOR_SCOPE() \
>   ScopedRequestSuspender pump_suspender__(mPump)
> 
> //-----------------------------------------------------------------------------
> // nsBaseChannel
> 
> nsBaseChannel::nsBaseChannel()
>   : mLoadFlags(LOAD_NORMAL)
>+  , mStatus(NS_OK)
>   , mQueriedProgressSink(PR_TRUE)
>   , mSynthProgressEvents(PR_FALSE)
>   , mWasOpened(PR_FALSE)
>   , mWaitingOnAsyncRedirect(PR_FALSE)
>-  , mStatus(NS_OK)
> {
>   mContentType.AssignLiteral(UNKNOWN_CONTENT_TYPE);
> }
> 
> nsresult
> nsBaseChannel::Redirect(nsIChannel *newChannel, PRUint32 redirectFlags,
>                         PRBool openNewChannel)
> {


what was this change about?  It probably should be in a different bug (maybe mark this bug as being blocked by it).


>+++ b/netwerk/dns/nsDNSService2.cpp
>@@ -243,19 +242,29 @@ nsDNSAsyncRequest::OnLookupComplete(nsHo
>     // the caller to be able to addref/release multiple times without
>     // destroying the record prematurely.
>     nsCOMPtr<nsIDNSRecord> rec;
>     if (NS_SUCCEEDED(status)) {
>         NS_ASSERTION(hostRecord, "no host record");
>         rec = new nsDNSRecord(hostRecord);
>         if (!rec)
>             status = NS_ERROR_OUT_OF_MEMORY;
>+#ifdef MOZ_ENABLE_QTNETWORK
>+    mListener->OnLookupComplete(this, rec, status);
>+    }
>+    else
>+    {
>+        if (!hostRecord->resolving)
>+        {
>+            mListener->OnLookupComplete(this, rec, status);
>+        }
>+#else
>     }
>-
>     mListener->OnLookupComplete(this, rec, status);
>+#endif

Whitespace looks off here. and I do not understand the need to make this change.  Comment please.

>@@ -497,16 +501,20 @@ nsHostResolver::ResolveHost(const char  
>                             PRUint16               flags,
>                             PRUint16               af,
>                             nsResolveHostCallback *callback)
> {
>     NS_ENSURE_TRUE(host && *host, NS_ERROR_UNEXPECTED);
> 
>     LOG(("nsHostResolver::ResolveHost [host=%s]\n", host));
> 
>+#ifdef MOZ_ENABLE_QTNETWORK
>+    if(!nsQtNetworkManager::instance()->EmitOpenConnection(QString(host)))
>+        return NS_ERROR_ABORT; //ABORT EVERYTHING!
>+#endif

Remove comment.  Change to NS_ERROR_UNKNOWN_HOST?
#if defined(RES_RETRY_ON_FAILURE)
>         if (!ai && rs.Reset())
>             ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> #endif
>-
>+#ifdef MOZ_ENABLE_QTNETWORK
>+        if (!ai)
>+          ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
>+#endif


why?  Shouldn't you just define RES_RETRY_ON_FAILURE?


>+++ b/netwerk/dns/nsHostResolver.h
>@@ -116,24 +116,29 @@ public:
>                                 Negative cache entries are valid just like any other
>                                 (though never for more than 60 seconds), but a use
>                                 of that negative entry forces an asynchronous refresh. */
> 
>     PRUint32     expiration; /* measured in minutes since epoch */
> 
>     PRBool HasResult() const { return addr_info || addr || negative; }
> 
>+#ifdef MOZ_ENABLE_QTNETWORK   /* true if this record is being resolved, which means
>+    PRBool  resolving;         * that it is either on the pending queue or owned by
>+#endif                         * one of the worker threads. */
> private:
>     friend class nsHostResolver;
> 
>     PRCList callbacks; /* list of callbacks */
>
>+#ifndef MOZ_ENABLE_QTNETWORK
>     PRBool  resolving; /* true if this record is being resolved, which means
>                         * that it is either on the pending queue or owned by
>                         * one of the worker threads. */
>+#endif

umm  something is wrong here? Maybe just remove the ifdefing?

> void
>-nsQtNetworkManager::CloseConnection()
>+nsQtNetworkManager::enableInstance()
> {
>-    NS_WARNING("nsQtNetworkManager::CloseConnection() Not implemented by QtNetwork.");
>+    NS_ASSERTION(gInstance, "nsQtNetworkManager must be created before calling enableInstance");
>+    if(gInstance)
>+    {
>+        gEnableInstance = true;
>+    }
>+}

how is this method and gEnableInstance any different than just using gInstance?

>+nsQtNetworkManager::EmitOpenConnection(const QString &host)
> {
>-    return IsConnected();

This is a really interesting name for a method that doesn't always emit?

Does it block?

>+    //if openSession is called within 200 ms again, we block this attempt since
>+    //its mostlike a fault of the browser
>+    //reset and wait for another 200 ms to clear opening again.
>+    if (mBlockTimer.isActive())
>+    {
>+        mBlockTimer.stop();
>+        mBlockTimer.setSingleShot(true);
>+        mBlockTimer.start(200);

Why 200ms?  200ms is a pretty large value.  How would this be the "fault of the browser"?

Hard to know where this is going.  Put up that description and happy to review again!

Comment 3

[Jeremias Bosch (:jbos)]

Alright,

ok I just put some explanation here, Part of the patch actually includes fixes for Bug 535793, as I'm really no necko expert I ifdefed the changes done there to be only qt network.

Returning NS_ERROR_ABORT is explicitly not UNKNOWNHOST. The result should not be a unknownhost error page as it is the wrong reaction here. What should happen is that the entire attempt to load a URL is aborted, to keep the user able to just surf on the last page.

About this EmitOpenConnection. 

The background is, that it is only possible to create a connection from within a thread which has access to the gmainloop. But in our case we might request a connection form within the context of a network thread. 

All this Emit / Not Emit OpenConnection is meant to make sure that we always create a Connection from within the mainthread; while doing so the thead calling is blocked. Meaning: if we would emit from within the mainthread we would block ourself and have a deadlock.


About the 200 ms. It is easy for the user to actually cancel the connection request, the question is, what should cause opening the modal connection dialog again. You might want it only on "useraction" but there is no way to make a difference to a "nonuseraction". 

As example: Go to AwesomeView, see that the Icons are not always cached and the get loaded on the fly. Not having the 200 ms rule would mean that instantly after the user dismissed the one conndialog without creating a connection he would see a new one, and will never be able to close / leave the view. Basically 200 ms are working fine, because two (or more) attemps to create a connection after dismissing one dialog is in 9/10 cases caused by software and not by user.

Comment 4

[Jeremias Bosch (:jbos)]

Attachment: updated comments

Updated Version.

Comment 5

[Jeremias Bosch (:jbos)]

Attachment: Special adjustments on dns for meego / qt.

Second patch (DNS part).

Comment 6

[Jeremias Bosch (:jbos)]

Attachment: Fennec mobile.js adjustments for Meego

Update mobile.js for meego to disable DNS Prefetch / Caching. Meego internal DNS Server is very slow and report strange states. This cause that dns prefetching reports wrong ips for hosts.

Comment 7

[Jeremias Bosch (:jbos)]

Attachment: updated review comments

Selected wrong version of patch. Updated.

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 503475 [details] [diff] [review]
Fennec mobile.js adjustments for Meego

I'm ok with these, but I want Doug to make the final call

Comment 9

[Jeremias Bosch (:jbos)]

ping. Oleg - without at least patch

 https://bugzilla.mozilla.org/attachment.cgi?id=503518

Network on Meego is not gona work

Comment 10

[Jeremias Bosch (:jbos)]

of course you need to have the fennec path too.: 

 Fennec mobile.js adjustments for Meego   


This is pretty important. Current impl. of QNetwork is just broken / wrong.

Comment 11

[Oleg Romashin (:romaxa)]

Comment on attachment 503518 [details] [diff] [review]
updated review comments

You need 
MOZ_QT_CFLAGS in netwerk/base/src/Makefile.in, otherwise it will not compile.


> nsresult
> nsAutodial::DialDefault(const PRUnichar* hostName)
> {
>-  if (nsQtNetworkManager::OpenConnectionSync())
>+  if (nsQtNetworkManager::instance()->openConnection(QString::fromUtf16(hostName)))
>     return NS_OK;
use {}

>-
>+  
  ^^
Please remove all white spaces from your patch
>  * ***** END LICENSE BLOCK ***** */
>-

don't remove this

>+#include "nsQtNetworkManager.h"
> #include "nsQtNetworkLinkService.h"
> #include "nsCOMPtr.h"
> #include "nsIObserverService.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
>-#include "nsQtNetworkManager.h"
>+


>-static QNetworkConfigurationManager* sNetworkConfig = 0;
>+#include <QTime>
>+#include <QDebug>

don't use qDebug, use NS_WARNING instead


>-    if (!default_cfg.isValid())
>+    if(!gQtNetworkManager)
        ^ space is missing


>+
>+nsQtNetworkManager::nsQtNetworkManager(QObject *parent) :
                                        
style it
(QObject* parent) :

>+    if(current.elapsed() < 1000)
>+        qDebug() << "[WARNING] Connection Creation was to fast, something is not right.";
NS_WARNING

>+    static nsQtNetworkManager* gQtNetworkManager;
>+    QNetworkSession *networkSession;

use:
Type* varName;

r+ with style comments fixed

Comment 12

[Oleg Romashin (:romaxa)]

Comment on attachment 503518 [details] [diff] [review]
updated review comments

Again similar style issues, ws, braces...

But in general it looks good.
Tried it and it also works fine.

Comment 13

[Oleg Romashin (:romaxa)]

Attachment: combined patch with fixed style.

Comment 14

[Oleg Romashin (:romaxa)]

Attachment: Combined patch + style fixes

Fixed double ifdef in netwerk/dns/Makefile.in
removed executable permissions changes for modified files...
Jeremias could you install or setup your editor to which is identifying whitespaces and don't change permissions to the files?

Comment 15

[Oleg Romashin (:romaxa)]

Comment on attachment 517910 [details] [diff] [review]
Combined patch + style fixes

Dougt can we get an approval for this patch?
it modifying some non-Qt files but under QTNETWORK ifdefs.

Comment 16

[Oleg Romashin (:romaxa)]

Comment on attachment 517910 [details] [diff] [review]
Combined patch + style fixes

Jason could you double check this patch, it touch some necko files (with QTNETWORK ifdefs)

Comment 17

[Doug Turner (:dougt)]

Comment on attachment 517910 [details] [diff] [review]
Combined patch + style fixes

that is a pretty ugly change to nsDNSService2.cpp.

Comment 18

[Oleg Romashin (:romaxa)]

yep, that is definitely ugly, I'll try to double check this patch and kill possible not needed parts.

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Removed dns resolve stuff

I just removed dns resolve hacks, and found that networking works fine without those changes.

Patch is almost Qt only now.

Comment 20

[Jason Duell]

Comment on attachment 519281 [details] [diff] [review]
Removed dns resolve stuff

Some feedback:

I don't know the autodialler (or socket transport error handling) code very well, so I'm not the ideal person to review this (I've cc'd Patrick McManus, Honza Bambas, and bz in case any of them know this stuff better).  And I'm sick, so my ability to wrap my brain around it is limited.

That said, I've looked over this a bit, and here are some thoughts:

It's really not very clear from the bug description or patch what issue(s) are being solved here.  It sounds like there's been some API change in the Meego (vs Maemo?) API that no longer allows calling OpenConnectionSync on any thread: so now we need to arrange to call openSession on the main thread, from originating calls on the Socket Transport thread.  Correct?

Then we also seem to be solving/kludging DNS resolution issues.  These are the result of the openSession call taking a long time to boot the network on the Meego device?  And/or of the "first" DNS resolve request taking an inordinate amount of time for some reason.

Things I'd like to know:

1) How important is getting Meego working for Fennec 1.0?  Do we have devices out there already, or very soon?  This isn't marked as a blocker.

2) Should the entire socket transport thread (STT) be blocked while we wait for the main thread to call QTNetworkSession::open?  I see the maemo code seems to call something blocking from the STT, so I assume the answer is yes.  (Generally that's a Bad Thing, but I guess when the entire network is down it may be sensible) 

3) So we're using QT's "signal/slot" mechanism to do the cross-thread event handling here.  I assume this is compatible with our regular thread event model (i.e. QT's event loop is hooked into ours).  If not we'd better use our stuff.

4) The part of the patch I'm most confused about are the various time-based hacks:  it 1) retries DNS resolution because the "first try takes a really long time (up to 10 seconds)"; and 2) ignores calls to openSession that happen within 200ms of the previous one since "it's most likely an automatic connection attempt which was not successful or canceled 200ms ago."  These strike me as kludgy and possibly fragile.  It's possible they're the best we can do, but I'd like to be sure we don't have some deterministic way of knowing what the right thing to do is (such as when we can issue DNS requests w/o having UDP packets all get dropped) instead of winging it with magic time values.

5) The patch from comment 6 changed mobile.js "for meego to disable DNS Prefetch / Caching."  Is that still in the final patch?  If so, that sucks, and if we can't fix it now we should at least open a bug on it, as this will cause a perf hit.  (If the OS DNS resolver is getting swamped by too many requests, we could try lowering the max outstanding request count to see if it'll behave for some smaller number.)

If this is critical for the fennec release, and the mobile folks are confident that it's OK, I could be persuaded to +r it.  Otherwise I'd like to have someone who knows this code better do a review.  (dougt: if you don't feel comfortable with it, maybe give to Honza or Patrick?).

Minor nits below.

------------------------------------------------

> nsresult
> nsAutodial::DialDefault(const PRUnichar* hostName)
> {
>-  if (nsQtNetworkManager::OpenConnectionSync())
>+  if (nsQtNetworkManager::instance()->openConnection(QString::fromUtf16(hostName))) {
>     return NS_OK;
>+  }

If I understand dougt's suggestion in comment 2, the idea is to initialize gQtNetworkManager once (which you do in nsQtNetworkLinkService::Init(), assuming that's early enough), and then use gQtNetworkManager directly, rather than make repeated calls like this to instance().  Of course you'd need to make sure all uses follow nsQtNetworkLinkService::Init().  See gHttpHandler for an example of something similar.



>+#ifdef MOZ_ENABLE_QTNETWORK
>+    //Request a connection in case we are offline before resolving the name. This avoid dns caching issues
>+    //on meego. If this failes, we are definitly offline. We do not want to show any error page in this case. We just want to allow the
>+    //user to keep surfing on the current website.

80 columns max, please.  And all '//' comments should have a space after the '//', i.e. 

    // my comment

    not

    //my comment
    
This needs to be fixed all over the place.


>@@ -886,16 +898,23 @@ nsHostResolver::ThreadFunc(void *arg)
>             flags |= PR_AI_NOCANONNAME;
> 
>         ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> #if defined(RES_RETRY_ON_FAILURE)
>         if (!ai && rs.Reset())
>             ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> #endif
> 
>+#ifdef MOZ_ENABLE_QTNETWORK
>+        //the resolving attempt on meego is really slow on first try (up to 10 seconds)
>+        //give it a second chance in case the first one failed.
>+        if (!ai) {
>+            ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
>+        }
>+#endif

Assuming we want this reissue logic, if we have RES_RETRY_ON_FAILURE defined, do we really want to try a third time?  (ie should the QTNetwork code be within an #else block and only called if !RES_RETRY_ON_FAILURE).


>+/*
>+  This function is called from different threads, we need to make sure that
>+  the attempt to create a network connection is done in the mainthread

Prefer block comments with "//" on each line.  If you use multi-line, generally use the format in our IDL files, with a "*' at the start of each line, like

/* 
 * Calls Bar in order to notify the frob.
 * -- warning: not thread-safe.
 */

>+class nsQtNetworkManager : public QObject
> {
>+  Q_OBJECT
>+  public:
>+    virtual ~nsQtNetworkManager();
>+    static nsQtNetworkManager* instance();
>+    static void enableInstance();
>+    static void destroy();
>+    PRBool openConnection(const QString&);
>+    PRBool isOnline();
>+  signals:
>+    void openConnectionSignal();
> 
>+  public slots:
>+    void closeSession();
>+    void onlineStateChanged(bool);
>+
>+  private slots:
>+    void openSession();

So you're not following our general naming convention (class methods start with uppercase.  Is there some reason for this?  If not, s/instance/Instance/, etc.

>+  private:
>+    explicit nsQtNetworkManager(QObject* parent = 0);
>+    static nsQtNetworkManager* gQtNetworkManager;
>+    QNetworkSession* networkSession;
>+    QNetworkConfiguration networkConfiguration;
>+    QNetworkConfigurationManager networkConfigurationManager;
>+    QTimer mBlockTimer;
>+    bool mOnline;

Similarly, member variables should use the "mFoo" style, which you're not using consistently.

Comment 21

[Jason Duell]

Comment on attachment 519281 [details] [diff] [review]
Removed dns resolve stuff

By the way, re: hacking DNS resolution.  You're removed the hack in nsDNSService2, but kept something similar in nsHostResolver.cpp.   Did you mean to remove it there too?

Comment 22

[Jeremias Bosch (:jbos)]

Background for DNS Resolving is a concept issue which is already appearing in Maemo 5. (see bugs about device goes online but shows offline / host error page)
  
The device reports it is "online"; but this is misunderstood. Maemo and Meego devices run a local dns server to allow having in resolv.conf "localhost" as nameserver. In case a Network Interface gets setup the dns server setups network specific (operator specific) dns masks. This takes time. 

Online means -> network interface up. But until the dns server is setuped correctly you will get the "unknown host" errors. There is no signal or notice which tells you "really online - network can be used now". You need to wait approx. 5 seconds after online - but this depends to network, network type, operator, how many programs run on the device currently, current memory usage,...

I was talking with the responsible architect and developers of several aspects in this issue. (connectivity impl., dns,...)

1) this bug is also present in Maemo 5. It got not addressed because people expected first to have on newer hardware a faster dns mask setup. -> this is not the case.
2) for meego they at least planed (time xmas) to implement a workaround to report a more reasonable online state. (basically waiting for successful dns mask setup.)
3) in general dns resolving on mobile networks (e.g gsm) is very slow. Esp directly after setup of a network. Mostlike you will loose the first and even the second request. Time for those - if they work- are ~8 and ~5 seconds.

The hacks in dnsservice2 are working around an to early try in using the network. In other words we try to access network even before we successfully resolved the hostname. This leads to a error page "unknown host".

What the hack does is, it asks "are we still resolving?" -> yes, than we wait. Yes this decrease the pageloading speed, but it avoids the error page.

I totally agree that its not the correct solution, if it works currently in meego without the hacks than I expect that the system reports "online" in a more reasonable way now. (but the bug is still in maemo5)

The fix here, and why it works without dnsservice2 is because the patch covers two cases.

A) resolving of a host when entering a new URL (changes in nsHostResolver)
B) reuse of a existing transfer (download, videostream) and changing of the network on the fly. (changes in dnsservice2)

While for A the entire resolve business is on hold when the first host need to get resolved, there is even a extra qt host resolving going on for beeing sure. You need to  think about situations of loosing network while having active channels - they will run into errors and will try to recover from them -> case B.

I hope its a bit clearer now. I know this topic is complex, and I do not want to say the hack is the real fix. But it works and was tested for several months without any complains.

Comment 24

[Jeremias Bosch (:jbos)]

Comment on attachment 519281 [details] [diff] [review]
Removed dns resolve stuff

Not working to reactivate a broken channel. (like streaming)

Comment 25

[Jeremias Bosch (:jbos)]

Comment on attachment 517910 [details] [diff] [review]
Combined patch + style fixes

With this patch it does work to reactive a broken channel (streaming).

Comment 26

[Oleg Romashin (:romaxa)]

ok, it is a bit hard to review and understand patch which is fixing many different problems at the same time like (connection up, connection timeout, dns resolving, streaming, dns fetch et.c.)

I think it would be easier to review and fix this bug if separate problems will be handled in separate bugs with explanation of:
1) problem
2) platform
3) testcase

only in that case we going to have clear understanding what why and how... and fix this bug step by step without 3 pages of comments.

Comment 27

[Patrick McManus [:mcmanus]]

Comment on attachment 503475 [details] [diff] [review]
Fennec mobile.js adjustments for Meego


> #ifdef MOZ_PLATFORM_MAEMO
> pref("network.autodial-helper.enabled", true);
>+pref("network.manage-offline-status", true);
>+/*disable prefetch due to issues on resolving hosts directly after changing networks in meego 
>+(system is slow 10-15 seconds on setting internal DNS masks but report it self online!)*/
>+pref("network.dns.disablePrefetch", true);

dns prefetch is ideal for the high latencies of mobile - you don't just want to disable it because of this network activation issue.. let's figure out how to make them play nicely togeher.

I feel like you need some kind of "call me back when you're confident the network is _really_ up" routine

Comment 28

[Patrick McManus [:mcmanus]]

Comment on attachment 519281 [details] [diff] [review]
Removed dns resolve stuff

I'm really only commenting on the hostresolver changes in here, because that's the only code that is patched that I really understand well.

>@@ -497,16 +501,24 @@ nsHostResolver::ResolveHost(const char  
>                             PRUint16               flags,
>                             PRUint16               af,
>                             nsResolveHostCallback *callback)
> {
>     NS_ENSURE_TRUE(host && *host, NS_ERROR_UNEXPECTED);
> 
>     LOG(("nsHostResolver::ResolveHost [host=%s]\n", host));
> 
>+#ifdef MOZ_ENABLE_QTNETWORK
>+    //Request a connection in case we are offline before resolving the name. This avoid dns caching issues
>+    //on meego. If this failes, we are definitly offline. We do not want to show any error page in this case. We just want to allow the
>+    //user to keep surfing on the current website.
>+    if (!nsQtNetworkManager::instance()->openConnection(QString(host))) {
>+        return NS_ERROR_ABORT; //ABORT EVERYTHING!
>+    }
>+#endif

Why is this hooked in DNS? Simply because that is an early stage in an HTTP transaction? Perhaps some place like nsHttpConnectionMgr::EnsureSocketThreadTargetIfOnline() makes more sense? Even then we probably want something more generic so that other folks like QT can register their own hooks.

>@@ -886,16 +898,23 @@ nsHostResolver::ThreadFunc(void *arg)
>             flags |= PR_AI_NOCANONNAME;
> 
>         ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> #if defined(RES_RETRY_ON_FAILURE)
>         if (!ai && rs.Reset())
>             ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> #endif
> 
>+#ifdef MOZ_ENABLE_QTNETWORK
>+        //the resolving attempt on meego is really slow on first try (up to 10 seconds)
>+        //give it a second chance in case the first one failed.
>+        if (!ai) {
>+            ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
>+        }
>+#endif

I understand the motivation, but I don't think double lookups of every failure on meego is the way to go. Especially as reuses of negative lookups trigger background retries anyhow. Maybe a test that says "if !ai and I haven't had any successful lookups in at least N seconds, then retry" would be workable without even a qt specific ifdef.




>         // convert error code to nsresult.
>         nsresult status = ai ? NS_OK : NS_ERROR_UNKNOWN_HOST;
>         resolver->OnLookupComplete(rec, status, ai);
>         LOG(("lookup complete for %s ...\n", rec->host));
>     }
>     NS_RELEASE(resolver);
>     LOG(("nsHostResolver::ThreadFunc exiting\n"));
> }
>diff --git a/netwerk/dns/nsHostResolver.h b/netwerk/dns/nsHostResolver.h
>--- a/netwerk/dns/nsHostResolver.h
>+++ b/netwerk/dns/nsHostResolver.h
>@@ -123,18 +123,18 @@ public:
> 
> private:
>     friend class nsHostResolver;
> 
>     PRCList callbacks; /* list of callbacks */
> 
>     PRBool  resolving; /* true if this record is being resolved, which means
>                         * that it is either on the pending queue or owned by
>-                        * one of the worker threads. */ 
>-    
>+                        * one of the worker threads. */
>+
>     PRBool  onQueue;  /* true if pending and on the queue (not yet given to getaddrinfo())*/
>     PRBool  usingAnyThread; /* true if off queue and contributing to mActiveAnyThreadCount */
>     
> 
>    ~nsHostRecord();
> };
> 
> /**
>diff --git a/netwerk/system/qt/Makefile.in b/netwerk/system/qt/Makefile.in
>--- a/netwerk/system/qt/Makefile.in
>+++ b/netwerk/system/qt/Makefile.in
>@@ -43,20 +43,24 @@ VPATH		= @srcdir@
> 
> include $(DEPTH)/config/autoconf.mk
> 
> MODULE         = necko
> LIBRARY_NAME   = neckosystem_s
> LIBXUL_LIBRARY = 1
> 
> FORCE_STATIC_LIB = 1
>+MOCSRCS = \
>+		moc_nsQtNetworkManager.cpp \
>+		$(NULL)
> 
> CPPSRCS += \
>+	$(MOCSRCS) \
> 	nsQtNetworkLinkService.cpp \
> 	nsQtNetworkManager.cpp \
> 	$(NULL)
> 
> include $(topsrcdir)/config/rules.mk
> 
> DEFINES += -DIMPL_NS_NET
> 
>-OS_INCLUDES += $(GLIB_CFLAGS) $(MOZ_QT_CFLAGS)
>+OS_INCLUDES += $(MOZ_QT_CFLAGS)
> LOCAL_INCLUDES += -I$(srcdir)/../../base/src
>diff --git a/netwerk/system/qt/nsQtNetworkLinkService.cpp b/netwerk/system/qt/nsQtNetworkLinkService.cpp
>--- a/netwerk/system/qt/nsQtNetworkLinkService.cpp
>+++ b/netwerk/system/qt/nsQtNetworkLinkService.cpp
>@@ -30,77 +30,92 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>+#include "nsQtNetworkManager.h"
> #include "nsQtNetworkLinkService.h"
> #include "nsCOMPtr.h"
> #include "nsIObserverService.h"
> #include "nsServiceManagerUtils.h"
> #include "nsString.h"
>-#include "nsQtNetworkManager.h"
> #include "mozilla/Services.h"
> 
> NS_IMPL_ISUPPORTS2(nsQtNetworkLinkService,
>                    nsINetworkLinkService,
>                    nsIObserver)
> 
> nsQtNetworkLinkService::nsQtNetworkLinkService()
> {
> }
> 
> nsQtNetworkLinkService::~nsQtNetworkLinkService()
> {
> }
> 
> NS_IMETHODIMP
>-nsQtNetworkLinkService::GetIsLinkUp(PRBool *aIsUp)
>+nsQtNetworkLinkService::GetIsLinkUp(PRBool* aIsUp)
> {
>-  *aIsUp = nsQtNetworkManager::IsConnected();
>+  *aIsUp = nsQtNetworkManager::instance()->isOnline();
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>-nsQtNetworkLinkService::GetLinkStatusKnown(PRBool *aIsKnown)
>+nsQtNetworkLinkService::GetLinkStatusKnown(PRBool* aIsKnown)
> {
>-  *aIsKnown = nsQtNetworkManager::GetLinkStatusKnown();
>+  *aIsKnown = nsQtNetworkManager::instance()->isOnline();
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsQtNetworkLinkService::Observe(nsISupports* aSubject,
>                                 const char* aTopic,
>                                 const PRUnichar* aData)
> {
>-  if (!strcmp(aTopic, "xpcom-shutdown"))
>+  if (!strcmp(aTopic, "xpcom-shutdown")) {
>     Shutdown();
>+    nsQtNetworkManager::instance()->destroy();
>+  }
>+
>+  if (!strcmp(aTopic, "browser-lastwindow-close-granted")) {
>+    Shutdown();
>+  }
> 
>   return NS_OK;
> }
> 
> nsresult
> nsQtNetworkLinkService::Init(void)
> {
>   nsCOMPtr<nsIObserverService> observerService =
>     mozilla::services::GetObserverService();
>-  if (!observerService)
>+  if (!observerService) {
>     return NS_ERROR_FAILURE;
>+  }
> 
>-  nsresult rv = observerService->AddObserver(this, "xpcom-shutdown", PR_FALSE);
>-  if (NS_FAILED(rv))
>+  nsresult rv;
>+
>+  rv = observerService->AddObserver(this, "xpcom-shutdown", PR_FALSE);
>+  if (NS_FAILED(rv)) {
>     return NS_ERROR_FAILURE;
>+  }
> 
>-  if (!nsQtNetworkManager::Startup())
>+  rv = observerService->AddObserver(this, "browser-lastwindow-close-granted", PR_FALSE);
>+  if (NS_FAILED(rv)) {
>     return NS_ERROR_FAILURE;
>+  }
>+
>+  //create singleton
>+  nsQtNetworkManager::instance();
> 
>   return NS_OK;
> }
> 
> nsresult
> nsQtNetworkLinkService::Shutdown()
> {
>-  nsQtNetworkManager::Shutdown();
>+  nsQtNetworkManager::instance()->closeSession();
>   return NS_OK;
> }
>diff --git a/netwerk/system/qt/nsQtNetworkManager.cpp b/netwerk/system/qt/nsQtNetworkManager.cpp
>--- a/netwerk/system/qt/nsQtNetworkManager.cpp
>+++ b/netwerk/system/qt/nsQtNetworkManager.cpp
>@@ -30,108 +30,168 @@
>  * use your version of this file under the terms of the MPL, indicate your
>  * decision by deleting the provisions above and replace them with the notice
>  * and other provisions required by the GPL or the LGPL. If you do not delete
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
>-//order is important - mozilla redefine qt macros
>-#include <QNetworkConfigurationManager>
>-#include <QNetworkConfiguration>
>-#include <QNetworkSession>
>-
> #include "nsQtNetworkManager.h"
> 
> #include "nsCOMPtr.h"
> #include "nsThreadUtils.h"
>-
> #include "nsINetworkLinkService.h"
>-
> #include "nsIOService.h"
> #include "nsIObserverService.h"
> #include "nsIOService.h"
> 
>-#include "nsINetworkLinkService.h"
>+#include <QHostInfo>
>+#include <QHostAddress>
>+#include <QTime>
> 
>-static QNetworkConfigurationManager* sNetworkConfig = 0;
>+nsQtNetworkManager* nsQtNetworkManager::gQtNetworkManager = nsnull;
> 
>-PRBool
>-nsQtNetworkManager::OpenConnectionSync()
>+//----------------------
>+
>+nsQtNetworkManager*
>+nsQtNetworkManager::instance()
> {
>-    if (!sNetworkConfig)
>-        return PR_FALSE;
>-
>-    //do not request when we are online...
>-    if (sNetworkConfig->isOnline())
>-        return PR_FALSE;
>-
>-    if (!(sNetworkConfig->capabilities() & QNetworkConfigurationManager::CanStartAndStopInterfaces))
>-        return PR_FALSE;
>-
>-    // Is there default access point, use it
>-    QNetworkConfiguration default_cfg = sNetworkConfig->defaultConfiguration();
>-
>-    if (!default_cfg.isValid())
>-    {
>-        NS_WARNING("default configuration is not valid. Looking for any other:");
>-        foreach (QNetworkConfiguration cfg, sNetworkConfig->allConfigurations())
>-        {
>-            if (cfg.isValid())
>-                default_cfg = cfg;
>-        }
>-
>-        if (!default_cfg.isValid())
>-        {
>-            NS_WARNING("No valid configuration found. Giving up.");
>-            return PR_FALSE;
>-        }
>+    if (!gQtNetworkManager) {
>+        gQtNetworkManager = new nsQtNetworkManager();
>+        connect(gQtNetworkManager, SIGNAL(openConnectionSignal()), gQtNetworkManager, SLOT(openSession()), Qt::BlockingQueuedConnection);
>+        connect(&gQtNetworkManager->networkConfigurationManager, SIGNAL(onlineStateChanged(bool)), gQtNetworkManager, SLOT(onlineStateChanged(bool)));
>     }
> 
>-    //do use pointer here, it will be deleted after connected!
>-    //Creation on stack cause appearing issues and segfaults of the connectivity dialog
>-    QNetworkSession* session = new QNetworkSession(default_cfg);
>-    QObject::connect(session, SIGNAL(opened()),
>-                     session, SLOT(deleteLater()));
>-    QObject::connect(session, SIGNAL(error(QNetworkSession::SessionError)),
>-                     session, SLOT(deleteLater()));
>-    session->open();
>-    return session->waitForOpened(-1);
>+    return gQtNetworkManager;
> }
> 
> void
>-nsQtNetworkManager::CloseConnection()
>+nsQtNetworkManager::destroy()
> {
>-    NS_WARNING("nsQtNetworkManager::CloseConnection() Not implemented by QtNetwork.");
>+    if (gQtNetworkManager) {
>+        delete gQtNetworkManager;
>+    }
>+    gQtNetworkManager = nsnull;
>+}
>+
>+nsQtNetworkManager::nsQtNetworkManager(QObject* parent)
>+  : QObject(parent), networkSession(0)
>+{
>+    mOnline = networkConfigurationManager.isOnline();
>+    NS_ASSERTION(NS_IsMainThread(), "nsQtNetworkManager can only initiated in Main Thread");
>+}
>+
>+nsQtNetworkManager::~nsQtNetworkManager()
>+{
>+    closeSession();
>+    networkSession->deleteLater();
> }
> 
> PRBool
>-nsQtNetworkManager::IsConnected()
>+nsQtNetworkManager::isOnline()
> {
>-    NS_ASSERTION(sNetworkConfig, "Network not initialized");
>-    return sNetworkConfig->isOnline();
>-}
>-
>-PRBool
>-nsQtNetworkManager::GetLinkStatusKnown()
>-{
>-    return IsConnected();
>-}
>-
>-PRBool
>-nsQtNetworkManager::Startup()
>-{
>-    //Dont do it if already there
>-    if (sNetworkConfig)
>-        return PR_FALSE;
>-
>-    sNetworkConfig = new QNetworkConfigurationManager();
>-
>-    return PR_TRUE;
>+    static PRBool sForceOnlineUSB = getenv("MOZ_MEEGO_NET_ONLINE") != 0;
>+    return sForceOnlineUSB || mOnline;
> }
> 
> void
>-nsQtNetworkManager::Shutdown()
>+nsQtNetworkManager::onlineStateChanged(bool online)
> {
>-    delete sNetworkConfig;
>-    sNetworkConfig = nsnull;
>+    mOnline = online;
> }
>+
>+/*
>+  This function is called from different threads, we need to make sure that
>+  the attempt to create a network connection is done in the mainthread
>+
>+  In case this function is called by another thread than the mainthread 
>+  we emit a signal which is connected through "BlockingQueue"-Connection Type.
>+
>+  This cause that the current thread is blocked and waiting for the result.
>+
>+  Of course, in case this gets called from the mainthread we must not emit the signal,
>+  but call the slot directly.
>+*/
>+
>+PRBool
>+nsQtNetworkManager::openConnection(const QString& host)
>+{
>+    //we are already online -> return true.
>+    if (isOnline()) {
>+        return true;
>+    }
>+
>+    if (NS_IsMainThread()) {
>+        openSession();
>+    } else {
>+        //jump to mainthread and do the work there
>+        emit openConnectionSignal();
>+    }
>+
>+    //if its claiming its online -> send one resolve request ahead.
>+    //this is important because on mobile the first request can take up to 10 seconds
>+    //sending here one will help to avoid trouble and timeouts later
>+    if (isOnline()) {
>+        QHostInfo::fromName(host);
>+    }
>+
>+    return isOnline();
>+}
>+
>+void nsQtNetworkManager::openSession()
>+{
>+    if (mBlockTimer.isActive()) {
>+        //if openSession is called within 200 ms again, we forget this attempt since
>+        //its mostlike an automatic connection attempt which was not successful or canceled 200ms ago.
>+        //we reset the timer and start it again.
>+
>+        //As example: Go in firefox mobile into AwesomeView, see that the icons are not always cached and
>+        //get loaded on the fly. Not having the 200 ms rule here would mean that instantly
>+        //after the user dismissed the one connection dialog once another
>+        //would get opened. The user will never be able to close / leave the view until each such attempt is gone through.
>+
>+        //Basically 200 ms are working fine, its huge enough for automatic attempts to get covered and small enough to 
>+        //still be able to react on direct user request.
>+
>+        mBlockTimer.stop();
>+        mBlockTimer.setSingleShot(true);
>+        mBlockTimer.start(200);
>+        return;
>+    }
>+
>+    if (isOnline()) {
>+        return;
>+    }
>+
>+    //this only means we did not shutdown before...
>+    //renew Session every time
>+    //fix/workaround for prestart bug
>+    if (networkSession != 0) {
>+        networkSession->close();
>+        networkSession->deleteLater();
>+    }
>+
>+    //renew always to react on changed Configurations
>+    networkConfigurationManager.updateConfigurations();
>+    //go always with default configuration
>+    networkConfiguration = networkConfigurationManager.defaultConfiguration();
>+    networkSession = new QNetworkSession(networkConfiguration);
>+
>+    networkSession->open();
>+    QTime current;
>+    current.start();
>+    networkSession->waitForOpened(-1);
>+
>+    if (current.elapsed() < 1000) {
>+        NS_WARNING("Connection Creation was to fast, something is not right.");
>+    }
>+
>+    mBlockTimer.setSingleShot(true);
>+    mBlockTimer.start(200);
>+}
>+
>+void nsQtNetworkManager::closeSession()
>+{
>+    if (networkSession != 0) {
>+        networkSession->close();
>+    }
>+}
>diff --git a/netwerk/system/qt/nsQtNetworkManager.h b/netwerk/system/qt/nsQtNetworkManager.h
>--- a/netwerk/system/qt/nsQtNetworkManager.h
>+++ b/netwerk/system/qt/nsQtNetworkManager.h
>@@ -33,26 +33,47 @@
>  * the provisions above, a recipient may use your version of this file under
>  * the terms of any one of the MPL, the GPL or the LGPL.
>  *
>  * ***** END LICENSE BLOCK ***** */
> 
> #ifndef NSQTNETWORKMANAGER_H_
> #define NSQTNETWORKMANAGER_H_
> 
>+#include <QNetworkConfigurationManager>
>+#include <QObject>
>+#include <QTimer>
>+#include <QNetworkConfiguration>
>+#include <QNetworkSession>
> #include "nscore.h"
> 
>-class nsQtNetworkManager
>+class nsQtNetworkManager : public QObject
> {
>-public:
>-  // Can be called from any thread, most likely the socket transport thread
>-  static PRBool OpenConnectionSync();
>-  static void CloseConnection();
>+  Q_OBJECT
>+  public:
>+    virtual ~nsQtNetworkManager();
> 
>-  static PRBool IsConnected();
>-  static PRBool GetLinkStatusKnown();
>+    static nsQtNetworkManager* instance();
>+    static void enableInstance();
>+    static void destroy();
>+    PRBool openConnection(const QString&);
>+    PRBool isOnline();
>+  signals:
>+    void openConnectionSignal();
> 
>-  // Called from the nsQtNetworkLinkService (main thread only)
>-  static PRBool Startup();
>-  static void Shutdown();
>+  public slots:
>+    void closeSession();
>+    void onlineStateChanged(bool);
>+
>+  private slots:
>+    void openSession();
>+
>+  private:
>+    explicit nsQtNetworkManager(QObject* parent = 0);
>+    static nsQtNetworkManager* gQtNetworkManager;
>+    QNetworkSession* networkSession;
>+    QNetworkConfiguration networkConfiguration;
>+    QNetworkConfigurationManager networkConfigurationManager;
>+    QTimer mBlockTimer;
>+    bool mOnline;
> };
> 
> #endif /* NSQTNETWORKMANAGER_H_ */

Comment 29

[Patrick McManus [:mcmanus]]

sorry for the trailing useless huge quote in comment 28. Wish I could edit that.

Comment 30

[Jeremias Bosch (:jbos)]

i started to split the patch up

Minimal needed changes for qt api useage.

https://bugzilla.mozilla.org/show_bug.cgi?id=646396

Comment 31

[Jeremias Bosch (:jbos)]

Enable to even use network management

https://bugzilla.mozilla.org/show_bug.cgi?id=646408

Comment 32

[Jeremias Bosch (:jbos)]

Patches from here take care about establishing and reestablishing of a connection
 
https://bugzilla.mozilla.org/show_bug.cgi?id=646417
https://bugzilla.mozilla.org/show_bug.cgi?id=646412

Comment 33

[Jeremias Bosch (:jbos)]

> >@@ -497,16 +501,24 @@ nsHostResolver::ResolveHost(const char  
> >                             PRUint16               flags,
> >                             PRUint16               af,
> >                             nsResolveHostCallback *callback)
> > {
> >     NS_ENSURE_TRUE(host && *host, NS_ERROR_UNEXPECTED);
> > 
> >     LOG(("nsHostResolver::ResolveHost [host=%s]\n", host));
> > 
> >+#ifdef MOZ_ENABLE_QTNETWORK
> >+    //Request a connection in case we are offline before resolving the name. This avoid dns caching issues
> >+    //on meego. If this failes, we are definitly offline. We do not want to show any error page in this case. We just want to allow the
> >+    //user to keep surfing on the current website.
> >+    if (!nsQtNetworkManager::instance()->openConnection(QString(host))) {
> >+        return NS_ERROR_ABORT; //ABORT EVERYTHING!
> >+    }
> >+#endif
> 
> Why is this hooked in DNS? Simply because that is an early stage in an HTTP
> transaction? Perhaps some place like
> nsHttpConnectionMgr::EnsureSocketThreadTargetIfOnline() makes more sense? Even
> then we probably want something more generic so that other folks like QT can
> register their own hooks.


I tried working around there, but i found it most usefull and best working place to ensure we are connected before even trying to resolve a host (host resolving without connection on non local host does not make sense)



> 
> >@@ -886,16 +898,23 @@ nsHostResolver::ThreadFunc(void *arg)
> >             flags |= PR_AI_NOCANONNAME;
> > 
> >         ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> > #if defined(RES_RETRY_ON_FAILURE)
> >         if (!ai && rs.Reset())
> >             ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> > #endif
> > 
> >+#ifdef MOZ_ENABLE_QTNETWORK
> >+        //the resolving attempt on meego is really slow on first try (up to 10 seconds)
> >+        //give it a second chance in case the first one failed.
> >+        if (!ai) {
> >+            ai = PR_GetAddrInfoByName(rec->host, rec->af, flags);
> >+        }
> >+#endif
> 
> I understand the motivation, but I don't think double lookups of every failure
> on meego is the way to go. Especially as reuses of negative lookups trigger
> background retries anyhow. Maybe a test that says "if !ai and I haven't had any
> successful lookups in at least N seconds, then retry" would be workable without
> even a qt specific ifdef.
>

Doublechecked -> not needed anymore. Removed it.

Comment 34

[Oleg Romashin (:romaxa)]

Comment on attachment 503471 [details] [diff] [review]
Special adjustments on dns for meego / qt.

this is obsolete, and currently in other bug

Comment 35

[BMO Automation]

Closing all opened bug in a graveyard component