807501 - Add proper console logs when app cache manifest load failes

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Honza Bambas (:mayhemer)]

I on and on receive bug mails where people are asking why the hell their offline app cache is not offline cached.  To figure out I usually have to process logs.

Hence, I will add proper logs when an error occurs during manifest load or processing as warnings or infos in the error console for:

- manifest has not been changed, not updating cache [info]
- manifest removed from the server, cache cleared [info]
- manifest request failed with error X, cache load failed [warning]
- manifest failed to parse on item N, cache load failed [warning]
- manifest item N failed to load with error X, cache load failed [warning]
- manifest item N is a redirect, cache load failed [warning]
- manifest N updated successfully [info]

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Honza Bambas (:mayhemer) from comment #0)
>  [...] in the error console [...]:

AFAICT, such errors should be put into the web console, not the error console. One reason is private browsing. The other reason is the long-term goal is for the error console to go away (the menu item for it has already been removed, recently). The most compelling reason, though, is that developers expect networking error messages for the current tab to be in the web console.

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #1)
> (In reply to Honza Bambas (:mayhemer) from comment #0)
> >  [...] in the error console [...]:
> 
> AFAICT, such errors should be put into the web console, not the error
> console. One reason is private browsing. The other reason is the long-term
> goal is for the error console to go away (the menu item for it has already
> been removed, recently). The most compelling reason, though, is that
> developers expect networking error messages for the current tab to be in the
> web console.

I was expecting nsIConsoleService logged to web console, but it's not.

Better would be to just add offline cache update requests to the load group and that would automatically log them all to the web console what may give enough clue.  

I just work on that, since logging is not what I actually want finally. (I may close this as WONTFIX eventually).

Comment 3

[Honza Bambas (:mayhemer)]

Adding all requests to the document's load group is not the way here.  When user cancels the load or just changes the URL (we also do this in offline tests) then offline cache loads are canceled and the update process fails.  That is against the spec.  Offline cache loads also must not influence the page load indicator and e.g. mixed content or security UI.

Brian, do you know how to log to the Web Console?  I cannot find out how its API works.  I have a patch to log to Error Console, I think we can change it simply to log to Web Console later.

I'll submit the patch and make it to land to have at least something quickly now.

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 5

[Josh Matthews [:jdm]]

Web consoles look for nsIScriptError objects that have window IDs that are non-zero.

Comment 6

[Josh Matthews [:jdm]]

In other words, use ReportScriptError instead of LogStringMessage and pass a relevant window ID to the script error Init method.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to Josh Matthews [:jdm] from comment #6)
> In other words, use ReportScriptError instead of LogStringMessage and pass a
> relevant window ID to the script error Init method.

Necko channels shouldn't know anything about windows.

I suggest that we add a logging interface that the notification callbacks can implement. Then, have nsDocShell implement that logging interface by getting the window ID and then ReportScriptError with it. AFAICT, this will allow any/all developer tools and addons to hook into the logging to report it where necessary.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to Brian Smith (:bsmith) from comment #7)
> (In reply to Josh Matthews [:jdm] from comment #6)
> > In other words, use ReportScriptError instead of LogStringMessage and pass a
> > relevant window ID to the script error Init method.
> 
> Necko channels shouldn't know anything about windows.
> 

Brian, I want to add the logs to the offline cache update code that knows (has to) windows.

> I suggest that we add a logging interface that the notification callbacks
> can implement. Then, have nsDocShell implement that logging interface by
> getting the window ID and then ReportScriptError with it. AFAICT, this will
> allow any/all developer tools and addons to hook into the logging to report
> it where necessary.

I don't agree with anything complicated.  I want right now a simple way to see what happens.  Rather then wait until somebody (because I don't have time) finish the machine you propose.  Report a new bug for that please.

Comment 9

[Honza Bambas (:mayhemer)]

(In reply to Josh Matthews [:jdm] from comment #6)
> In other words, use ReportScriptError instead of LogStringMessage and pass a
> relevant window ID to the script error Init method.

What API do you have in mind?

Comment 10

[Josh Matthews [:jdm]]

I wasn't thinking; I forgot that nsIScriptError inherits from nsIConsoleMessage. The nsIConsoleService stuff works fine.

Comment 11

[Jason Duell]

Comment on attachment 677445 [details] [diff] [review]
v1

Review of attachment 677445 [details] [diff] [review]:
-----------------------------------------------------------------

::: uriloader/prefetch/nsOfflineCacheUpdate.cpp
@@ +81,5 @@
>      uint32_t mCount;
>      char **mValues;
>  };
>  
> +namespace { // anon

I really dislike anonymous namespaces.  See my rant on dev.platform.  I'd guess the result will be that you should feel free to use them :)

Comment 12

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #11)
> I really dislike anonymous namespaces.  

I don't like so many things in our styling guild...

RE namespaces: use visual studio ;)

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 677445 [details] [diff] [review]
v1

https://hg.mozilla.org/integration/mozilla-inbound/rev/b97258fb92ba

I left the namespace.  If something changes in this manner, then it will be a big replace anyway.

Comment 14

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/b97258fb92ba

Comment 15

[Jason Duell]

Comment on attachment 677445 [details] [diff] [review]
v1

Just adds some debugging console infrastructure, which is used by b2g blocker bug 761040

Comment 16

[Jason Duell]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/b63d9e25cb6f

Comment 17

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #16)
> https://hg.mozilla.org/releases/mozilla-b2g18/rev/b63d9e25cb6f

Thanks!  I didn't realize it didn't ever land on b2g18.