Mojo: Fix two bugs in content handling

Mojo: Fix two bugs in content handling

1. URLLoaders can't be reused, so we need to construct one
   per-load. This became too gnarly to handle with callbacks
   so I put it back to having a loader object that owns the
   URLLoader.

2. The relevant URLLoader needs to be passed to 
   ContentHandler::OnConnect() so that it stays alive long 
   enough to read the entire response.

BUG=

Committed: https://crrev.com/54efb2353d1a90840fcfad2ca5b5b3a8c7883154
Cr-Commit-Position: refs/heads/master@{#292460}

[Aaron Boodman, 2014-08-27 00:54:40]

Patchset #1 (id:1) has been deleted

[Aaron Boodman, 2014-08-27 01:10:18]

aa@chromium.org changed reviewers:
+ tim@chromium.org

[Aaron Boodman, 2014-08-27 01:10:55]

Tim, I figured you might be good for this since you've been working in this area
recently. If you don't think you're a good match, let me know.

[darin (slow to review), 2014-08-27 03:22:04]

darin@chromium.org changed reviewers:
+ darin@chromium.org

[darin (slow to review), 2014-08-27 03:22:04]

https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/content_handler/content_handler.mojom:10: struct
ContentHandlerResponse {
hmm..... this reminds me of the DocumentLoader object in blink. that object is
also called a WebDataSource at the blink API layer. just food for thought re:
naming.

https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:33: // while the async operation is
oustanding.
nit: oustanding -> outstanding

[Aaron Boodman, 2014-08-27 05:43:51]

https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
mojo/services/public/interfaces/content_handler/content_handler.mojom:10: struct
ContentHandlerResponse {
On 2014/08/27 03:22:04, darin wrote:
> hmm..... this reminds me of the DocumentLoader object in blink. that object is
> also called a WebDataSource at the blink API layer. just food for thought re:
> naming.

Doesn't really do anything for me, but if you want me to change it, I will.

https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:33: // while the async operation is
oustanding.
On 2014/08/27 03:22:04, darin wrote:
> nit: oustanding -> outstanding

Done.

[darin (slow to review), 2014-08-27 06:17:37]

On 2014/08/27 05:43:51, Aaron Boodman wrote:
>
https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
> File mojo/services/public/interfaces/content_handler/content_handler.mojom
> (right):
> 
>
https://codereview.chromium.org/513573002/diff/40001/mojo/services/public/int...
> mojo/services/public/interfaces/content_handler/content_handler.mojom:10:
struct
> ContentHandlerResponse {
> On 2014/08/27 03:22:04, darin wrote:
> > hmm..... this reminds me of the DocumentLoader object in blink. that object
is
> > also called a WebDataSource at the blink API layer. just food for thought
re:
> > naming.
> 
> Doesn't really do anything for me, but if you want me to change it, I will.

No need. I didn't have a concrete suggestion. Just thinking out loud...


> 
>
https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
> File mojo/shell/dynamic_application_loader.cc (right):
> 
>
https://codereview.chromium.org/513573002/diff/40001/mojo/shell/dynamic_appli...
> mojo/shell/dynamic_application_loader.cc:33: // while the async operation is
> oustanding.
> On 2014/08/27 03:22:04, darin wrote:
> > nit: oustanding -> outstanding
> 
> Done.

[darin (slow to review), 2014-08-27 06:34:19]

LGTM

https://codereview.chromium.org/513573002/diff/60001/mojo/application_manager...
File mojo/application_manager/application_loader.h (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/application_manager...
mojo/application_manager/application_loader.h:37: URLResponsePtr url_response,
Did you considered using ContentHandlerResponsePtr here as well? Maybe the extra
level of indirection would be annoying?

https://codereview.chromium.org/513573002/diff/60001/mojo/services/public/int...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/513573002/diff/60001/mojo/services/public/int...
mojo/services/public/interfaces/content_handler/content_handler.mojom:9: 
ContentSource perhaps?

My motivation for thinking about different names:
1- ContentHandlerResponse is kind of long
2- ContentHandlerResponse leads to awkward |response->response| code

(this choice doesn't need to block this CL... just food for thought really)

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:35: internal::LoaderDelegate* delegate)
what was the purpose of introducing LoaderDelegate? was it to avoid having to
make Loader and subclasses friends of DynamicApplicationLoader?

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:228: } else {
nit: no need for "else" after "return"

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.h (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.h:29: // Please do not use, this is an
implementation detail.
it would be possible to hide this by moving most of the state of
DynamicApplicationLoader into an inner class, but that's probably not worth the
effort.

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.h:37: virtual ~LoaderDelegate() {
nit: list dtor first as that's pretty canonical.

[tim (not reviewing), 2014-08-27 17:32:19]

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:30: // DynamicApplicationLoader going
away (and taking all the Loaders with it)
I'm not sure I follow why this isn't enough in terms of ownership (sorry, I
think I'm just missing something). The Loader shouldn't be destroyed until the
app is done, so why do we care about something that might still be reading a
response from the URLLoader at that point?

[Aaron Boodman, 2014-08-27 18:26:13]

https://codereview.chromium.org/513573002/diff/60001/mojo/application_manager...
File mojo/application_manager/application_loader.h (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/application_manager...
mojo/application_manager/application_loader.h:37: URLResponsePtr url_response,
On 2014/08/27 06:34:19, darin wrote:
> Did you considered using ContentHandlerResponsePtr here as well? Maybe the
extra
> level of indirection would be annoying?

I did consider that... I didn't do it because it didn't really cut down on
duplication. Callers already have a |url_response| and a |url_loader|. Making
them pass ContentHandlerResponsePtr just forces them to write extra code to
create the struct. It also felt a little weird to have the loaders have to
include content_handler.mojom.h.

It's a minor thing though, I'll change it if you like.

https://codereview.chromium.org/513573002/diff/60001/mojo/services/public/int...
File mojo/services/public/interfaces/content_handler/content_handler.mojom
(right):

https://codereview.chromium.org/513573002/diff/60001/mojo/services/public/int...
mojo/services/public/interfaces/content_handler/content_handler.mojom:9: 
On 2014/08/27 06:34:19, darin wrote:
> ContentSource perhaps?
> 
> My motivation for thinking about different names:
> 1- ContentHandlerResponse is kind of long
> 2- ContentHandlerResponse leads to awkward |response->response| code
> 
> (this choice doesn't need to block this CL... just food for thought really)

I like ContentSource, thanks. Changed.

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:35: internal::LoaderDelegate* delegate)
On 2014/08/27 06:34:19, darin wrote:
> what was the purpose of introducing LoaderDelegate? was it to avoid having to
> make Loader and subclasses friends of DynamicApplicationLoader?

Yes, I considered using friend, but there sure ends up being a lot of
friendliness going on (there are like 5 or 6 things that Loader and subclasses
need to access from DynamicApplicationLoader).

The other option is to pass Loader's dependencies into its constructor. I've
implemented it like this in the latest patch, so you can take a look.

The downside is that there ends up being a lot of dependencies and they all are
weak pointers back to DynamicApplicationLoader... so I think it's maybe slightly
more complex to understand the lifetime issues.

But the upshot is that it gets rid of the public delegate interface...

Let me know what you think.

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.h (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.h:37: virtual ~LoaderDelegate() {
On 2014/08/27 06:34:19, darin wrote:
> nit: list dtor first as that's pretty canonical.

irrelevant in latest patch.

[Aaron Boodman, 2014-08-27 18:27:37]

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:35: internal::LoaderDelegate* delegate)
On 2014/08/27 18:26:13, Aaron Boodman wrote:
> On 2014/08/27 06:34:19, darin wrote:
> > what was the purpose of introducing LoaderDelegate? was it to avoid having
to
> > make Loader and subclasses friends of DynamicApplicationLoader?
> 
> Yes, I considered using friend, but there sure ends up being a lot of
> friendliness going on (there are like 5 or 6 things that Loader and subclasses
> need to access from DynamicApplicationLoader).
> 
> The other option is to pass Loader's dependencies into its constructor. I've
> implemented it like this in the latest patch, so you can take a look.
> 
> The downside is that there ends up being a lot of dependencies and they all
are
> weak pointers back to DynamicApplicationLoader... so I think it's maybe
slightly
> more complex to understand the lifetime issues.
> 
> But the upshot is that it gets rid of the public delegate interface...
> 
> Let me know what you think.

Sorry s/weak pointers/raw pointers/.

[Aaron Boodman, 2014-08-27 18:33:43]

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:30: // DynamicApplicationLoader going
away (and taking all the Loaders with it)
On 2014/08/27 17:32:19, timsteele wrote:
> I'm not sure I follow why this isn't enough in terms of ownership (sorry, I
> think I'm just missing something). The Loader shouldn't be destroyed until the
> app is done, so why do we care about something that might still be reading a
> response from the URLLoader at that point?

I was worried about the case where DynamicApplicationLoader itself is destroyed
while a URLLoader or a file read is happening. I figure this can happen during
system shutdown..?

[darin (slow to review), 2014-08-27 19:57:53]

LGTM

[tim (not reviewing), 2014-08-27 22:54:14]

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/60001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:30: // DynamicApplicationLoader going
away (and taking all the Loaders with it)
On 2014/08/27 18:33:43, Aaron Boodman wrote:
> On 2014/08/27 17:32:19, timsteele wrote:
> > I'm not sure I follow why this isn't enough in terms of ownership (sorry, I
> > think I'm just missing something). The Loader shouldn't be destroyed until
the
> > app is done, so why do we care about something that might still be reading a
> > response from the URLLoader at that point?
> 
> I was worried about the case where DynamicApplicationLoader itself is
destroyed
> while a URLLoader or a file read is happening. I figure this can happen during
> system shutdown..?

Yes but I don't see why it matters. In the system shutdown case, the
DynamicApplicationLoader is owned by the ApplicationManager, and the AM is
destroyed after the shell message loop has quit. If the shell message loop has
quit, then any connected applications should have (or will soon) receive a call
(Terminate) that will kill their message loops. I'd expect the ContentHandler to
go away as well?

[Aaron Boodman, 2014-08-27 23:51:11]

On Wed, Aug 27, 2014 at 3:54 PM, <tim@chromium.org> wrote:

>
> https://codereview.chromium.org/513573002/diff/60001/mojo/
> shell/dynamic_application_loader.cc
> File mojo/shell/dynamic_application_loader.cc (right):
>
> https://codereview.chromium.org/513573002/diff/60001/mojo/
> shell/dynamic_application_loader.cc#newcode30
> mojo/shell/dynamic_application_loader.cc:30: // DynamicApplicationLoader
> going away (and taking all the Loaders with it)
> On 2014/08/27 18:33:43, Aaron Boodman wrote:
>
>> On 2014/08/27 17:32:19, timsteele wrote:
>> > I'm not sure I follow why this isn't enough in terms of ownership
>>
> (sorry, I
>
>> > think I'm just missing something). The Loader shouldn't be destroyed
>>
> until the
>
>> > app is done, so why do we care about something that might still be
>>
> reading a
>
>> > response from the URLLoader at that point?
>>
>
>  I was worried about the case where DynamicApplicationLoader itself is
>>
> destroyed
>
>> while a URLLoader or a file read is happening. I figure this can
>>
> happen during
>
>> system shutdown..?
>>
>
>

> Yes but I don't see why it matters. In the system shutdown case, the
> DynamicApplicationLoader is owned by the ApplicationManager, and the AM
> is destroyed after the shell message loop has quit. If the shell message
> loop has quit, then any connected applications should have (or will
> soon) receive a call (Terminate) that will kill their message loops. I'd
> expect the ContentHandler to go away as well?
>

This is why I wanted you to review this :). Thanks for the thoughtful
review...

The case I'm worried about is where app A has asked for a connection to app
B, and DSL has started loading app B in response, but app B hasn't actually
started running yet. We're still fetching the bits for app B
asynchronously.

If shell ML is stopped *after* the AM is destroyed, then isn't it possible
that we'll end up pumping a few more messages after AM is gone, and thus a
Loader subclass will try to poke at AM after it's dead?


> https://codereview.chromium.org/513573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tim (not reviewing), 2014-08-28 01:17:49]

> 
> This is why I wanted you to review this :). Thanks for the thoughtful
> review...
> 
> The case I'm worried about is where app A has asked for a connection to app
> B, and DSL has started loading app B in response, but app B hasn't actually
> started running yet. We're still fetching the bits for app B
> asynchronously.
> 
> If shell ML is stopped *after* the AM is destroyed, then isn't it possible
> that we'll end up pumping a few more messages after AM is gone, and thus a
> Loader subclass will try to poke at AM after it's dead?
> 

[my first code review comment sent from a plane! sorry about the delay]

The shell ML is quit *before* the AM is destroyed. If you look at
https://codereview.chromium.org/491443005/ you see that the Context was made a
delegate of ApplicationManager for the purpose of observing applications going
away and deciding at some point (when the primordial apps are gone) to Quit()
the MessageLoop.  That's step one. The Context (and hence ApplicationManager)
won't be destroyed until later (Note also the DCHECK(!MessageLoop::current()) in
~Context). 

The other goal of that patch was to give DynamicApplicationLoader direct
ownership of the runners...  So I think the idea in your case was that A would
be told to Terminate(), and then the loader (and runner if it existed) for B
would be destroyed by ~AM before OnLoadComplete. OnLoadComplete is weakly bound
to the Loader, and we'll never pump any more shell messages...

[Aaron Boodman, 2014-08-28 02:49:47]

On Wed, Aug 27, 2014 at 6:17 PM, <tim@chromium.org> wrote:

>
>
>  This is why I wanted you to review this :). Thanks for the thoughtful
>> review...
>>
>
>  The case I'm worried about is where app A has asked for a connection to
>> app
>> B, and DSL has started loading app B in response, but app B hasn't
>> actually
>> started running yet. We're still fetching the bits for app B
>> asynchronously.
>>
>
>  If shell ML is stopped *after* the AM is destroyed, then isn't it possible
>>
>> that we'll end up pumping a few more messages after AM is gone, and thus a
>> Loader subclass will try to poke at AM after it's dead?
>>
>
>
> [my first code review comment sent from a plane! sorry about the delay]
>
> The shell ML is quit *before* the AM is destroyed. If you look at
> https://codereview.chromium.org/491443005/ you see that the Context was
> made a
> delegate of ApplicationManager for the purpose of observing applications
> going
> away and deciding at some point (when the primordial apps are gone) to
> Quit()
> the MessageLoop.  That's step one. The Context (and hence
> ApplicationManager)
> won't be destroyed until later (Note also the
> DCHECK(!MessageLoop::current()) in
> ~Context).
>
> The other goal of that patch was to give DynamicApplicationLoader direct
> ownership of the runners...  So I think the idea in your case was that A
> would
> be told to Terminate(), and then the loader (and runner if it existed) for
> B
> would be destroyed by ~AM before OnLoadComplete. OnLoadComplete is weakly
> bound
> to the Loader, and we'll never pump any more shell messages...
>

I see. So it sounds like you are saying I don't actually need the callbacks
to Loader to use WeakPtrs.

It still sort of seems worthwhile to do it though... because conceptually,
the callbacks are to the Loader instance, and they should never run after
the Loader dies. Relying on the fact that the message loop goes away at the
same time seems non-idea.

I'll find you online tomorrow to talk about this.


>
> https://codereview.chromium.org/513573002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Aaron Boodman, 2014-08-28 18:58:56]

OK, I hashed it out with Tim... He was pointing out that you don't need to pass
URLLoader to content handler implementations because the URLLoader is kept alive
until the app is done by DynamicApplicationLoader (along with the rest of the
corresponding Loader subclass).

So I was able to remove the ContentSource struct completely and put OnConnect()
back to just receiving a URLResponse.

[tim (not reviewing), 2014-08-28 19:11:14]

Yup, thanks for bearing with me. LGTM with nit.

[For posterity, I know this means we'll keep the URLLoader around longer, but it
seems better than having to pass two things around just to read from one of them
even if we abstract it with ContentSource-like things. I prefer the more basic
mojom API. 

If we later decide that we care about the overhead (note the previous code was
also keeping it around), I feel like it would be better solved at the
URLLoader/Response level by providing a way for you to say "i'm done with this
loader when i'm done with the response". If that's common behavior it's awkward
to have everyone define their own ContentSource-like thing IMO.]

https://codereview.chromium.org/513573002/diff/80001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:67: base::Bind(&Loader::AppCompleted,
weak_ptr_factory_.GetWeakPtr()));
nit: I kind of liked the RunLibrary / OnRunLibraryComplete naming symmetry vs
AppCompleted.

[Aaron Boodman, 2014-08-28 19:49:23]

https://codereview.chromium.org/513573002/diff/80001/mojo/shell/dynamic_appli...
File mojo/shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/513573002/diff/80001/mojo/shell/dynamic_appli...
mojo/shell/dynamic_application_loader.cc:67: base::Bind(&Loader::AppCompleted,
weak_ptr_factory_.GetWeakPtr()));
On 2014/08/28 19:11:14, timsteele wrote:
> nit: I kind of liked the RunLibrary / OnRunLibraryComplete naming symmetry vs
> AppCompleted.

Yeah, but it is run in other situations than when running of library is
complete.

[Aaron Boodman, 2014-08-28 19:49:27]

The CQ bit was checked by aa@chromium.org

[commit-bot: I haz the power, 2014-08-28 19:51:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/aa@chromium.org/513573002/150001

[commit-bot: I haz the power, 2014-08-28 20:51:39]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  ios_dbg_simulator on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)

[commit-bot: I haz the power, 2014-08-28 21:01:52]

Committed patchset #8 (id:150001) as 8cf11e1b4ae186269ba8128800651134292c158e

[commit-bot: I haz the power, 2014-09-10 03:02:11]

Patchset 8 (id:??) landed as
https://crrev.com/54efb2353d1a90840fcfad2ca5b5b3a8c7883154
Cr-Commit-Position: refs/heads/master@{#292460}