968417 - revalidating images without a loadgroup

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[Patrick McManus [:mcmanus]]

imglib appears to intentionally revalidate images without using the loadgroup of the triggering request because the load can be shared and we don't want cancel events on one loadgroup to cancel the load.

otoh necko uses the loadgroup to tie some things together into a logical set for scheduling and caching purposes.

non-validating image loads work around this by creating a new loadgroup and setting the parent to the original one.

I'm going to include a patch that does the same thing for validations.

Absence of the loadgroup has lead to some http2 and spdy push failures in https://http2.iijplus.jp/public/push/push_test.html

Comment 1

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=8e6d2770d1f4

Comment 2

[Patrick McManus [:mcmanus]]

Attachment: spdy/http-2 server push fails on image revalidation

Comment 3

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8387765 [details] [diff] [review]
spdy/http-2 server push fails on image revalidation

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

::: image/src/imgLoader.cpp
@@ +1211,5 @@
> +    nsCOMPtr<nsILoadGroupChild> childLoadGroup = do_QueryInterface(loadGroup);
> +    if (childLoadGroup) {
> +      childLoadGroup->SetParentLoadGroup(aLoadGroup);
> +    }
> +    newChannel->SetLoadGroup(loadGroup);

So this all looks good, but I have to wonder why we don't just do this in NewImageChannel. There are two callers: this one (ValidateRequestWithNewChannel) and LoadImage. Given the reasoning in comment 0, it sounds like we don't really want to use a null loadgroup in LoadImage either. Is there a reason why the situation in LoadImage is different?

::: netwerk/protocol/http/Http2Stream.cpp
@@ +315,5 @@
>        pushedStream = cache->RemovePushedStreamHttp2(hashkey);
>  
> +    LOG3(("Pushed Stream Lookup "
> +          "session=%p key=%s loadgroupci=%p cache=%p hit=%p\n",
> +          mSession, hashkey.get(), loadGroupCI, cache, pushedStream));

Just making sure this was intended to be a part of this patch.

Comment 4

[Patrick McManus [:mcmanus]]

(In reply to Seth Fowler [:seth] from comment #3)
> Comment on attachment 8387765 [details] [diff] [review]
> spdy/http-2 server push fails on image revalidation
> 
> Review of attachment 8387765 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/imgLoader.cpp
> @@ +1211,5 @@
> > +    nsCOMPtr<nsILoadGroupChild> childLoadGroup = do_QueryInterface(loadGroup);
> > +    if (childLoadGroup) {
> > +      childLoadGroup->SetParentLoadGroup(aLoadGroup);
> > +    }
> > +    newChannel->SetLoadGroup(loadGroup);
> 
> So this all looks good, but I have to wonder why we don't just do this in
> NewImageChannel. There are two callers: this one
> (ValidateRequestWithNewChannel) and LoadImage. Given the reasoning in
> comment 0, it sounds like we don't really want to use a null loadgroup in
> LoadImage either. Is there a reason why the situation in LoadImage is
> different?


loadimage has the same logic (around line 1702).. I can consoldiate it in newimagechannel if you like.


> Just making sure this was intended to be a part of this patch.

yes.. its useful logging for the issue the patch is addressing.

thanks!

Comment 5

[Seth Fowler [:seth] [:s2h]]

(In reply to Patrick McManus [:mcmanus] from comment #4)
> loadimage has the same logic (around line 1702).. I can consoldiate it in
> newimagechannel if you like.

Yeah, I'd definitely prefer that if it's straightforward.

Comment 6

[Patrick McManus [:mcmanus]]

Attachment: revalidating images needs loadgroup parent for spdy push

Comment 7

[Patrick McManus [:mcmanus]]

https://tbpl.mozilla.org/?tree=Try&rev=ef2d9fbd3860

I also removed an unused variable: newChannel. (I actually got confused by it and momentarily used it instead of *aResult)

Comment 8

[Seth Fowler [:seth] [:s2h]]

Comment on attachment 8399409 [details] [diff] [review]
revalidating images needs loadgroup parent for spdy push

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

So good. Thanks, Patrick.

Comment 9

[Patrick McManus [:mcmanus]]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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