[Offline pages] Add null pointer checks in offline internals code

[Offline pages] Add null pointer checks in offline internals code

BUG=640294
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0fce96caaf852df8e616a922ef814d94f9bc7ca8
Cr-Commit-Position: refs/heads/master@{#416409}

[chili, 2016-08-23 23:56:27]

chili@chromium.org changed reviewers:
+ dbeam@chromium.org, dewittj@chromium.org

[Dan Beam, 2016-08-24 01:27:23]

https://codereview.chromium.org/2274733002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:176: if
(offline_page_model_)
all of these indents are off

[chili, 2016-08-24 01:31:17]

Thanks!

https://codereview.chromium.org/2274733002/diff/1/chrome/browser/ui/webui/off...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/1/chrome/browser/ui/webui/off...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:176: if
(offline_page_model_)
On 2016/08/24 01:27:23, Dan Beam wrote:
> all of these indents are off

For some reason, I was thinking Java @.@

Fixed!

[Dan Beam, 2016-08-24 01:36:50]

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:306:
offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile);
can you just call profile->GetOriginalProfile()?

[dewittj, 2016-08-24 02:02:57]

lgtm

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:306:
offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile);
On 2016/08/24 01:36:50, Dan Beam wrote:
> can you just call profile->GetOriginalProfile()?

I think the idea is that the factory will decide based on the profile that it is
given whether there is a valid offline page model for that profile.  So to me it
makes more sense to just do a null check afterwards.

However, after this it might make sense to just show an error page if the model
etc are not available rather than the normal internals page.

[chili, 2016-08-24 04:19:01]

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:306:
offline_pages::OfflinePageModelFactory::GetForBrowserContext(profile);
On 2016/08/24 02:02:57, dewittj wrote:
> On 2016/08/24 01:36:50, Dan Beam wrote:
> > can you just call profile->GetOriginalProfile()?
> 
> I think the idea is that the factory will decide based on the profile that it
is
> given whether there is a valid offline page model for that profile.  So to me
it
> makes more sense to just do a null check afterwards.
> 
> However, after this it might make sense to just show an error page if the
model
> etc are not available rather than the normal internals page.

I don't think a null check afterwards will help either.  At this point in the
code, the UI page hasn't even loaded yet.  It's not until we call either
GetEventLogs, GetLoggingState, GetRequestQueue or GetStoredPages that we know
the ui is getting loaded.  However, the type of their return values don't lend
well to something like "black out the UI page with some error page"

I think having default values to these things work well for now.  It's not
presenting wrong information and we don't have to remember to update this should
we decide another different kind of profile won't work and/or offline pages will
work in incognito

[chili, 2016-08-30 18:14:36]

Ping

[dewittj, 2016-08-30 18:17:52]

still lgtm!

[chili, 2016-08-30 18:20:55]

On 2016/08/30 18:17:52, dewittj wrote:
> still lgtm!

^_^  waiting for dbeam@'s owners approval

[Dan Beam, 2016-08-31 20:21:55]

what does this page do for incognito profiles?  is it useful?  can we just
disable this page for incognito profiles if not?

[chili, 2016-09-01 00:57:28]

On 2016/08/31 20:21:55, Dan Beam wrote:
> what does this page do for incognito profiles?  is it useful?  can we just
> disable this page for incognito profiles if not?

From offline conversation -
It seems like the current way of preventing a page from showing up in incognito
does not work for android (they are compiled for chrome_non_mobile_sources
only).  On an android device, pages which will automatically redirect away to a
normal profile when in incognito mode (i.e. chrome://history) will instead show
an empty page.  We might be able to create a redirect thing for android, but I
feel like that's for another CL, if not a bit overkill for the issue we want to
solve...

[Dan Beam, 2016-09-01 01:05:59]

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:176: if
(offline_page_model_)
can we just do

  if (!offline_page_model_)
    return;

or something in here and HandleSetRecordRequestQueue?

should these message handlers be hit (i.e. is there some way to do this from the
UI while incognito?).  if not: maybe just leave the code as is

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:257:
ResolveJavascriptCallback(*callback_id, base::FundamentalValue(false));
can we do this higher?  why bother getting |url| or generating a GUID if it
doesn't matter to the response?

[chili, 2016-09-01 01:29:15]

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc
(right):

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:176: if
(offline_page_model_)
On 2016/09/01 01:05:58, Dan Beam wrote:
> can we just do
> 
>   if (!offline_page_model_)
>     return;
> 
> or something in here and HandleSetRecordRequestQueue?
> 
> should these message handlers be hit (i.e. is there some way to do this from
the
> UI while incognito?).  if not: maybe just leave the code as is

I think the only way to do this from UI would be if UI first sends an
"isIncognito?" message.  If true, then doesn't do anything and if false, then
calls everything else

On the other hand, I think we should have this too as an additional safeguard. 
While it's true that we don't expect non-incognito profiles to not have a page
model, there may be other features in play that causes this to error out (i.e.
some required flag is unset, etc).

https://codereview.chromium.org/2274733002/diff/20001/chrome/browser/ui/webui...
chrome/browser/ui/webui/offline/offline_internals_ui_message_handler.cc:257:
ResolveJavascriptCallback(*callback_id, base::FundamentalValue(false));
On 2016/09/01 01:05:58, Dan Beam wrote:
> can we do this higher?  why bother getting |url| or generating a GUID if it
> doesn't matter to the response?

Done.

[Dan Beam, 2016-09-01 01:40:23]

what about adding an:

  html_source->AddBoolean("incognito", profile->IsIncognito());

to here
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/offline/offline_...

then

  var incognito = loadTimeData.getBoolean('incognito');
  $('log-model-on').hidden = incognito;
  $('log-model-off').hidden = incognito;
  $('log-request-on').hidden = incognito;
  $('log-request-off').hidden = incognito;

or something along those lines?

[chili, 2016-09-02 00:36:07]

Description was changed from

==========
[Offline pages] Add null pointer checks in offline internals code

BUG=640294
==========

to

==========
[Offline pages] Add null pointer checks in offline internals code

BUG=640294
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[chili, 2016-09-02 00:36:25]

On 2016/09/01 01:40:23, Dan Beam wrote:
> what about adding an:
> 
>   html_source->AddBoolean("incognito", profile->IsIncognito());
> 
> to here
>
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/offline/offline_...
> 
> then
> 
>   var incognito = loadTimeData.getBoolean('incognito');
>   $('log-model-on').hidden = incognito;
>   $('log-model-off').hidden = incognito;
>   $('log-request-on').hidden = incognito;
>   $('log-request-off').hidden = incognito;
> 
> or something along those lines?

Done. PTAL

[Dan Beam, 2016-09-02 00:43:56]

when can the C++ now be hit with null members?  i.e. why do you need the C++
changes now?  are we guarding against users that call chrome.send() from the
console?

[chili, 2016-09-02 17:26:31]

On 2016/09/02 00:43:56, Dan Beam wrote:
> when can the C++ now be hit with null members?  i.e. why do you need the C++
> changes now?  are we guarding against users that call chrome.send() from the
> console?

C++ can be hit with null if one or more of the underlying features guarding the
offline model and the request queues are turned off.  We are basing whether to
turn on features on whether it's incognito rather than whether an offline model
& request queue exists for the profile....

[Dan Beam, 2016-09-02 22:04:11]

lgtm

[chili, 2016-09-02 22:06:42]

The CQ bit was checked by chili@chromium.org

[chili, 2016-09-02 22:06:42]

The patchset sent to the CQ was uploaded after l-g-t-m from dewittj@chromium.org
Link to the patchset: https://codereview.chromium.org/2274733002/#ps60001
(title: "incognito")

[chili, 2016-09-02 22:07:11]

Thanks for reviewing!

[commit-bot: I haz the power, 2016-09-02 22:07:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 00:30:12]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-09-03 00:34:04]

Description was changed from

==========
[Offline pages] Add null pointer checks in offline internals code

BUG=640294
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
[Offline pages] Add null pointer checks in offline internals code

BUG=640294
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/0fce96caaf852df8e616a922ef814d94f9bc7ca8
Cr-Commit-Position: refs/heads/master@{#416409}
==========

[commit-bot: I haz the power, 2016-09-03 00:34:05]

Patchset 4 (id:??) landed as
https://crrev.com/0fce96caaf852df8e616a922ef814d94f9bc7ca8
Cr-Commit-Position: refs/heads/master@{#416409}