530075 - Try to requesting a network connection before showing offline error page

Status: RESOLVED INVALID

(Core :: Networking, defect)

Description

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

Some platforms support requesting a network connection when the system is offline. By requesting a network connection we can avoid showing the user the offline network error page. Instead, we can acquire a network connection and display the intended resource.

Comment 1

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

The mobile Maemo platform is one such case

Comment 2

[Jeremias Bosch (:jbos)]

Attachment: Patch to fix the behaviour.

The idea is 1) remove the offline page and 2) to put a check in the beginning
of
DocShell::DoURILoad(...).  which open a autodial system dialog if true.

During this dialog fennec will wait for a result. When the result is a
established connection fennec will implicitly leave the offline mode and load
the page. If the result is that we are still offline fennec will load a
(partly) cached version of the page or if no cached version present fennec just
stay on the old page. 

As i wrote above, we do not want to show the offline page at all! Why? 

We think that the offline page just interrupts the user experience and confuse
him by using a commonly used terminology "offline mode" which is used in mobile
phones (i.e. n900) for a state where the network chip is turned off. Another
name for that is "flight mode". 

But the phones offline mode has nothing to do with the browser offline mode.
The offline mode in the browser is just an interal browser state which is
caused i.e. when the user has no phone network and leaving a wlan access area.
(of cource when the phone is in offline mode, the browser will it implicitly
too)

Another bad thing about the offline page is this: You cant display partly
cached websites. They will get loaded, visible for the user, from the cache and
when the loading is then finished the offline page will get displayed instat.
Thats because some parts of the site where not present in the cache. This is
such a big issue because most pages these dayes have elements which do not get
cached. This makes the complete concept about displaying cached websites in
most cases unusable for the offline usecase.

Comment 3

[Dão Gottwald [:dao]]

jeremias, you need to request review from an actual person.

Comment 4

[Jeremias Bosch (:jbos)]

Attachment: connectivity ui fix

Fixed patch.

Comment 6

[Aakash Desai [:aakashd]]

...from the duplicated bug...

Fennec initiates a connection when a new URL is entered in the addrss bar, but
does Not re-initiate the connection when Connection is disconnected or lost and
Fennec is already open and running. 
For example, Visit a webpage, Disconnect the connection, then return to Fennec
and click on a link in the Browser. The Firefox Offline page is shown,
eventhough network is available. 
Browser should automatically try to connect, and only if network is Not
available show the Offline page.

Reproducible: Always

Steps to Reproduce:
1. Open Fennec and visit a webpage (ie. twitter.com )
2. Switch to Network Connection manager, and Disconnect the network connection.
3. Switch back to Fennec, and click on a link on the page. (ie. About us link
on twitter.com )

Actual Results:  
Offline mode page is shown, "Firefox is currently in offline mode and can't
browse the web" with a button to Try again.

Expected Results:  
Network is re-established automatically, webpage is downloaded and displayed.
Only when there is No network connection available should the Offline page be
displayed.

Repeat these same steps with the default N900 Maemo Browser, and the connection
is automatically re-established. This is more convenient for the user.
1. Open Maemo Browser and visit a webpage, such as twitter.com 
2. Switch to Network Connection manager, and Disconnect the network connection.
3. Switch back to Maemo Browser, and click on a link on the page. (ie. About us
link on twitter.com )
Actual Result: Connection is automatically re-established, webpage is
displayed.

Comment 7

[Oleg Romashin (:romaxa)]

Ping-pong, Mark can you check this patch, do you like it? if not the give some comments.

Comment 8

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

Comment on attachment 421236 [details] [diff] [review]
connectivity ui fix

Jason, can you take a look at the necko parts of this patch?

Comment 9

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

Quick review from Boris over IRC:


bz: r- on the docshell part because the "check for offline protocols" thing is bogus
bz: should be using the protocol handler flag we have for this
bz: also not quite sure why the IsFrame check in there; should be documented
bz: and need to rev the service iid
bz: and ideally add to end, not to beginning

bz: with those bits fixed docshell stuff looks fine to me

Comment 10

[James Cook]

Comment on attachment 421236 [details] [diff] [review]
connectivity ui fix

I'd like to understand why nsIOService::SetOffline(PRBool offline) is being modified.  I'm doing some tests with xpcshell and this change is causing some problems when trying to create server sockets.

Comment 11

[timeless]

Comment on attachment 421236 [details] [diff] [review]
connectivity ui fix

offhand, i think adding conic ifdefs to docshell seems bad.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 421236 [details] [diff] [review]
connectivity ui fix

This already has r- from me; see comment 9.  And yes, the ifdef is pretty suspect too.

Comment 13

[Jeremias Bosch (:jbos)]

Attachment: Updated Version

Updates regarding the issues from the review

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 460665 [details] [diff] [review]
Updated Version

This didn't address most of my review comments.  If you have questions, ask; don't just post the same thing I already said is wrong and ask review again...  It just wastes everyone's time.

Comment 15

[Jeremias Bosch (:jbos)]

Sorry my fault, overlooked that and just updated the version to our newest internal. Thought it was fixed proper.

> bz: should be using the protocol handler flag we have for this
that should be fixed - don't know about any different than the scheme.

> bz: and need to rev the service iid
what do you mean with that?

bz: and ideally add to end, not to beginning
Just dont get this part, the idea is that we catch attempts for connections before they cause any loading at all

Comment 16

[Boris Zbarsky [:bzbarsky]]

> that should be fixed - don't know about any different than the scheme.

Then _ask_.  https://hg.mozilla.org/mozilla-central/annotate/7ef1437c9275/netwerk/base/public/nsIProtocolHandler.idl#l231 is what you want.

> what do you mean with that?

You need to change the interface id of the interface you're changing.

> Just dont get this part, the idea is that we catch attempts for connections

I meant put the new method at the end of the interface.  That way you're not going to break existing binary consumers due to changes in the vtable layout.

Note that above I'm also concerned about the ifdefs.  Why do we need those?  Is this not something we'd want to do on all platforms, at least in terms of calling into the network link service?

Comment 17

[Jeremias Bosch (:jbos)]

> 
> Note that above I'm also concerned about the ifdefs.  Why do we need those?  Is
> this not something we'd want to do on all platforms, at least in terms of
> calling into the network link service?

Actually no, this is just something about maemo 6/meego where we do not want to have the offline error page at all, but see the old page. In order to achieve this, it is important that the connection gets requested before.

Might be considered for other platforms but not implemented yet, so only maemo 6 for now.

Comment 18

[Boris Zbarsky [:bzbarsky]]

That doesn't answer my question.  The maemo-only effect is handled by the network link service on other platforms erroring out, right? So why do we need the ifdefs?

Comment 19

[Jeremias Bosch (:jbos)]

Ok, i dont have a strong mind on that, we can just remove them from 

DoURILoad, we should keep them on the error page

Comment 20

[Boris Zbarsky [:bzbarsky]]

"on the error page"?  You mean in DisplayLoadError?  I'd really like to avoid ANY platform ifdefs in this code if possible.  Why is the DisplayLoadError change needed?  What does it do?

Comment 21

[Jeremias Bosch (:jbos)]

basically avoid showing the offline-mode page for meego/maemo fennec. Will try to get comment from madhava.

Comment 22

[Jeremias Bosch (:jbos)]

Attachment: Updated Version

I have updated the patch the defines are still in. Since its not 100% clear to me how we proceed, still need to get the comment but madhava is on pert. leave. Need to find someone else from ui to comment on it.

Comment 23

[Jeremias Bosch (:jbos)]

will update the patch, just seen there is a debug output in there which i overlooked. but would be good to get your comment

Comment 24

[Boris Zbarsky [:bzbarsky]]

Again, why is the ifdef around the NS_RequestConnection stuff needed?

In any case, the right way to avoid showing the error page, _if_ that's what you really want to do is one of:

1)  Change things so the error coming through here is not
    NS_ERROR_DOCUMENT_NOT_CACHED.
2)  Introduce a preference that says what to do with
    NS_ERROR_DOCUMENT_NOT_CACHED errors.  Set the preference for your platform.

Hardcoded random platform ifdefs in cross-platform browser code are not the right way to do this, imo.

On to the other parts of the code:

*  You don't need the (void) before the NS_URIChainHasFlags call.  
*  I'm not sure I buy the subframe reasoning.  Wouldn't that break cases when a
   subframe has a meta refresh and you happen to have gone offline while the
   toplevel page is loaded?
*  I'm not sure what the comment about event loop means here.  If the Gecko
   event loop spins under that call, that's almost certainly not safe.  If this
   is some other random event loop... why do we care?
*  If the contract for requestConnection is that it must do whatever is needed
   synchronously before returning, that needs to be documented in the API, no?
*  Is polling at 1s intervals really superior to waiting on a condvar, say, and
   being woken up when things are ready to go?  Or is the point that
   gConnectionCallbackInvoked can only change while you're spinning the gtk
   event loop?

Comment 25

[Jeremias Bosch (:jbos)]

(In reply to comment #24)
> Again, why is the ifdef around the NS_RequestConnection stuff needed?
> 
Will remove it.

> In any case, the right way to avoid showing the error page, _if_ that's what
> you really want to do is one of:
> 
> 1)  Change things so the error coming through here is not
>     NS_ERROR_DOCUMENT_NOT_CACHED.
> 2)  Introduce a preference that says what to do with
>     NS_ERROR_DOCUMENT_NOT_CACHED errors.  Set the preference for your platform.
> 
> Hardcoded random platform ifdefs in cross-platform browser code are not the
> right way to do this, imo.
> 

Will do that, good point.

> On to the other parts of the code:
> 
> *  You don't need the (void) before the NS_URIChainHasFlags call.  
> *  I'm not sure I buy the subframe reasoning.  Wouldn't that break cases when a
>    subframe has a meta refresh and you happen to have gone offline while the
>    toplevel page is loaded?

that case is covered by the "normal" async connection request through necko "autodial api". This covers also cases like connection breakup during loading a page or downloading...

> *  I'm not sure what the comment about event loop means here.  If the Gecko
>    event loop spins under that call, that's almost certainly not safe.  If this
>    is some other random event loop... why do we care?

Its risky and it is only ok because of some assumptions/fakts. We had several discussions about this with mfinkle, doug, Christian Biesinger back last year. I hope to tell it in short.

The libconic lib works asynchron, attempts to provide a synchronous api where denied to keep backwards compatibility.

In order to achieve not to display any errorpage for the case we cancel the connection dialog, we needed to synchronize the request. Synchronizing this is really complicated since we are in the mainthread which process the gmainloop. Processing the gmainloop is needed to get the callback from lib in order to stop waiting.

This is scary and does only work because - the connection dialog is modal and must be modal as long as the connection establishing is ongoing. Leaving the dialog must always result the state as "connected" or "disconnected". 

With this assumptions in place its ok, because the user is not able to interact with the browser without leaving the dialog and go out of the blocking state.

> *  If the contract for requestConnection is that it must do whatever is needed
>    synchronously before returning, that needs to be documented in the API, no?

Will add some documentation

> *  Is polling at 1s intervals really superior to waiting on a condvar, say, and
>    being woken up when things are ready to go?  Or is the point that
>    gConnectionCallbackInvoked can only change while you're spinning the gtk
>    event loop?

see upper comment

Comment 26

[Boris Zbarsky [:bzbarsky]]

> that case is covered by the "normal" async connection request through necko
> "autodial api".

Hmm.  Why does that API not work for toplevel page loads?  What's special about subframes?

> With this assumptions in place its ok, because the user is not able to
> interact with the browser without leaving the dialog and go out of the
> blocking state.

And since we're only spinning the gtk event loop, not the Gecko one, we don't run into other sorts of events firing?  OK, that makes sense.  Thank you for explaining!

Comment 27

[Jeremias Bosch (:jbos)]

ok, i did a lot of tests and that patch is not needed at all anymore since it covers cases which are not in scope anymore -> resolve as invalid.