646412 - After Connection is established on MeeGo. Browser shows Error Page (Host not found)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Jeremias Bosch (:jbos)]

1. select connection
2. see it establish
3. see browser loading

Actuall Outcome:
Error page "Host not found" appears

Expected Outcome:
Page successfully loaded

----

When pressing retry page load correctly.

Comment 1

[Jeremias Bosch (:jbos)]

Attachment: Fix Version1

(you need patches form blocking bugs)

This patch creates a situation on MeeGo where we check if we are online 
before we even first resolve a Host.

In case we are offline, we synchronously go online and than resolve.

This avoid that a channel can fail, since we do not even create one.

Comment 2

[Jeremias Bosch (:jbos)]

Can we get a push on this? Its qt network only so there shouldn't be any side effect for others.

So it should be relative uncritical push.

Comment 3

[Oleg Romashin (:romaxa)]

Comment on attachment 522968 [details] [diff] [review]
Fix Version1

Patch need to be updated

Comment 4

[Jeremias Bosch (:jbos)]

Attachment: Updated Patch

Patch is updated to upstream

Comment 5

[Jeremias Bosch (:jbos)]

Attachment: Fixed to upstream use. also fixed crash in manager we found.

Comment 6

[Oleg Romashin (:romaxa)]

Comment on attachment 526729 [details] [diff] [review]
Fixed to upstream use. also fixed crash in manager we found.


>+#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 (gQtNetworkManager)
>+    {
why do you check for null here? gQtNetworkManager must not be null here, otherwise whole patch is just useless...

>+        if (!gQtNetworkManager->openConnection(QString(host))) {
>+            return NS_ERROR_ABORT; //Abort Everything, to not even show a errorpage. Just do nothing when connection failes.
>+        }
>+    }
>+#endif

Comment 7

[Jeremias Bosch (:jbos)]

Am i missing something?

(In reply to comment #6)

> >+    if (gQtNetworkManager)
> >+    {
> why do you check for null here? gQtNetworkManager must not be null here,
> otherwise whole patch is just useless...
> 

Point is: gQtNetworkManager is null in case you do not actually activate autodial but still have qt network enabled. We talked about this?!?

Comment 8

[Jeremias Bosch (:jbos)]

Attachment: Added comment to explain why check for 0 is needed

You need to check for 0 otherwise you might have a crash here. simple as that.

Comment 9

[Oleg Romashin (:romaxa)]

Comment on attachment 526945 [details] [diff] [review]
Added comment to explain why check for 0 is needed

Problem here is not about dial manager enabled/disabled... problem here is that you initializing static gQtNetworkManager in one place, and trying to use it in other place....
in nsHostResolver.cpp you will never get NON-NULL gQtNetworkManager, and with null check you are just hiding that problem.... 
With current patch network request session never opened, because gQtNetworkManager is always null

Comment 10

[Jeremias Bosch (:jbos)]

did you use patches from https://bugzilla.mozilla.org/show_bug.cgi?id=646408

Comment 11

[Jeremias Bosch (:jbos)]

I checked. Basically you need to have a singleton. it is just as simple as that. Since you do not want a singleton because of ugly syntax... i just think we run out of options here....

Comment 12

[Jeremias Bosch (:jbos)]

Attachment: Get it working

This works, please suggest something else working in case you don't like it.

Comment 13

[Oleg Romashin (:romaxa)]

Comment on attachment 527245 [details] [diff] [review]
Get it working


>+#ifdef MOZ_ENABLE_QTNETWORK
>+    //Request a connection in case we are offline before resolving the name. This avoid dns caching issues
        ^again problems with spaces in comments


>+    explicit nsQtNetworkManager(QObject* parent = 0);
>+
>+    static nsQtNetworkManager* gQtNetworkManager;

I guess this should be now not gQtNetworkManager, but "sQtNetworkManager"

Comment 14

[Oleg Romashin (:romaxa)]

Comment on attachment 527245 [details] [diff] [review]
Get it working

Patrik could you double check this once again.
probably nsHostResolver.cpp part

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 527245 [details] [diff] [review]
Get it working


> 
>     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::get()->openConnection(QString(host))) {
>+            return NS_ERROR_ABORT; //Abort Everything, to not even show a errorpage. Just do nothing when connection failes.
>+    }
>+#endif
>

I don't understand why this is in the DNS code. It doesn't seem to have anything to do with DNS. If you want to stop from making a connection of some sort, why don't you check this before making the channel rather than filling the DNS implementation with these odd platform specific details?

Convince me.

You say something about the cache in your comment. Is the issue that we should be clearing the cache when coming online and we aren't doing that? Or maybe that we should at least be clearing the negative cache entries and we aren't? (I cannot remember what we do in either case - I am just hypothesizing.)

Comment 16

[Jeremias Bosch (:jbos)]

> I don't understand why this is in the DNS code. It doesn't seem to have
> anything to do with DNS. If you want to stop from making a connection of some
> sort, why don't you check this before making the channel rather than filling
> the DNS implementation with these odd platform specific details?
> 
> Convince me.
> 

:) Lets try. This is there because it is the one common place which is triggered from different areas of the code and it is the last sane place to check if we are online. Resolving a name while we are offline will usually fail. On MeeGo it might not. -> MeeGo has its own DNS Server running, this returns IP addresses for hosts even when we are offline, since it does caching and prediction things. 

So you might be able to get an ip to a host while offline. This will fail as soon as you try to use the ip address. 

You will run into Bug 646417 and end up with an error page. To avoid all the extra work and steps going on in 646417 (meaning also to avoid the timeouts and reacting faster to user action) we check "online" before host gets even resolved.

Also in case user cancel the connection dialog we avoid here that he sees an error page and stays in the context. Normally he will see an error page and will lose the website he loaded before. With this approach he does not lose the website.

Comment 17

[Jeremias Bosch (:jbos)]

can we get this landed? I mean its really needed and it is really restricted.

Comment 18

[Oleg Romashin (:romaxa)]

No, this patch is definitely wrong, and we should not hack host resolver, because we have autodial stuff for that functionality.

I found problem why autodial pref does not work...
we have autodial pref observed before mSocketTransportService created, that is reason why autodial does not work

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: Fix autodial pref issue

Comment 20

[Oleg Romashin (:romaxa)]

Attachment: Updated and fixed Qt part

Ok, finally I got it working fine.
problem of current open session is that 
 connect(gQtNetworkManager, SIGNAL(openConnectionSignal()), gQtNetworkManager, SLOT(openSession()), Qt::BlockingQueuedConne
+        connect(&gQtNetworkManager->networkConfigurationManager, SIGNAL(onlineStateChanged(bool)), gQtNetworkManager, SLOT(onlin
was missing in previous network bug.

Comment 21

[Oleg Romashin (:romaxa)]

Comment on attachment 529839 [details] [diff] [review]
Fix autodial pref issue

ok, don't think double review needed here

Comment 22

[Oleg Romashin (:romaxa)]

Comment on attachment 529845 [details] [diff] [review]
Updated and fixed Qt part

wolfiR do you have time to look at this patch?

Comment 23

[Wolfgang Rosenauer [:wolfiR]]

Comment on attachment 529845 [details] [diff] [review]
Updated and fixed Qt part

Review of attachment 529845 [details] [diff] [review]:

Looks good to me.
Only one nit. The connect() calls should be wrapped I guess.

Comment 24

[Oleg Romashin (:romaxa)]

http://hg.mozilla.org/mozilla-central/rev/75a2043c0529
http://hg.mozilla.org/mozilla-central/rev/4a5bcd4057d7

Comment 25

[Anna (Waverley)]

I don't have a MeeGo phone.. can anyone verify this ?