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

RESOLVED FIXED

Status

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jbos, Assigned: romaxa)

Tracking

Trunk
x86
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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.
Posted patch Fix Version1 (obsolete) — Splinter Review
(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.
Attachment #522968 - Flags: review?(romaxa)
Depends on: 646396, 646408
Blocks: 609844
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 on attachment 522968 [details] [diff] [review]
Fix Version1

Patch need to be updated
Attachment #522968 - Flags: review?(romaxa)
Posted patch Updated Patch (obsolete) — Splinter Review
Patch is updated to upstream
Attachment #522968 - Attachment is obsolete: true
Attachment #526208 - Flags: review?(romaxa)
Attachment #526208 - Attachment is obsolete: true
Attachment #526208 - Flags: review?(romaxa)
Attachment #526729 - Flags: review?(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
Attachment #526729 - Flags: review?(romaxa) → review-
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?!?
You need to check for 0 otherwise you might have a crash here. simple as that.
Attachment #526729 - Attachment is obsolete: true
Attachment #526945 - Flags: review?(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
Attachment #526945 - Flags: review?(romaxa) → review-
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....
Posted patch Get it working (obsolete) — Splinter Review
This works, please suggest something else working in case you don't like it.
Attachment #526945 - Attachment is obsolete: true
Attachment #527245 - Flags: review?(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 on attachment 527245 [details] [diff] [review]
Get it working

Patrik could you double check this once again.
probably nsHostResolver.cpp part
Attachment #527245 - Flags: review?(romaxa)
Attachment #527245 - Flags: review?(mcmanus)
Attachment #527245 - Flags: review+
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.)
> 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.
can we get this landed? I mean its really needed and it is really restricted.
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
Attachment #529839 - Flags: review?
Attachment #529839 - Flags: review? → review?(mcmanus)
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.
Assignee: nobody → romaxa
Attachment #527245 - Attachment is obsolete: true
Attachment #527245 - Flags: review?(mcmanus)
Attachment #529845 - Flags: review?(doug.turner)
Attachment #529839 - Flags: review?(mcmanus) → review?(jduell.mcbugs)
Attachment #529839 - Flags: review?(cbiesinger)
Attachment #529839 - Flags: review?(cbiesinger) → review+
Comment on attachment 529839 [details] [diff] [review]
Fix autodial pref issue

ok, don't think double review needed here
Attachment #529839 - Flags: review?(jduell.mcbugs)
Comment on attachment 529845 [details] [diff] [review]
Updated and fixed Qt part

wolfiR do you have time to look at this patch?
Attachment #529845 - Flags: review?(mozilla)
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.
Attachment #529845 - Flags: review?(mozilla) → review+
http://hg.mozilla.org/mozilla-central/rev/75a2043c0529
http://hg.mozilla.org/mozilla-central/rev/4a5bcd4057d7
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
I don't have a MeeGo phone.. can anyone verify this ?
Attachment #529845 - Flags: review?(doug.turner)
You need to log in before you can comment on or make changes to this bug.