268177 - loadgroups don't handle loadgroups very well

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

- GetLoadGroup lacks an addref
- the loadgroup never adds itself to mLoadGroup

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[Darin Fisher]

Comment on attachment 164970 [details] [diff] [review]
patch

looks good, r=darin

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 164970 [details] [diff] [review]
patch

Hmmm... so without this patch cancelling a loadgroup for a document didn't
cancel loadgroups for child documents?	So an image loading in a frame would
not get cancelled when the toplevel document got cancelled (because we'd cancel
the subdocument's channel, but not its loadgroup)?

If so, then watch things carefully for regressions after this lands... (and
it's probably a good idea to check GetLoadGroup() callers in the tree to make
sure none of them were relying on the buggy behavior you fixed).

sr=bzbarsky.

Comment 4

[Darin Fisher]

bz: I don't think we ever added LoadGroups to other LoadGroups, so this is
likely to affect any existing consumers.  Or, I am mistaken?  I made
nsILoadGroup extend from nsIRequest relatively recently (w.r.t.
uriloader/docloader design).

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

indeed - http://lxr.mozilla.org/seamonkey/search?string=-%3Esetloadgroup only
shows channels that get put into loadgroups (and dummy requests). this is more
of something for use in the future.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Perhaps I was unclear...

Without this patch, a loadgroup (viewed as an nsIRequest) is not in any
loadgroup.  So the only way to cancel a loadgroup is to explicitly call Cancel()
on it.  At least as far as I can see.  Am I missing something?

With this patch, is a loadgroup placed in some parent loadgroup?  I guess that
only happens if someone called SetLoadGroup on the loadgroup.  I guess comment 5
is saying that currently that does not happen?

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

> So the only way to cancel a loadgroup is to explicitly call Cancel() on it. 

that's correct

>With this patch, is a loadgroup placed in some parent loadgroup?  I guess that
>only happens if someone called SetLoadGroup on the loadgroup.  I guess comment 5
>is saying that currently that does not happen?

right :)

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in netwerk/base/src/nsLoadGroup.cpp;
/cvsroot/mozilla/netwerk/base/src/nsLoadGroup.cpp,v  <--  nsLoadGroup.cpp
new revision: 1.63; previous revision: 1.62
done

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

oops. this caused a bit of bustage (RemoveRequest takes 3 args). I fixed it by
passing aStatus to removeRequest. that seems ok to me - it means the loadgroup
will indicate the status of the last removed request; it's all we have (isn't it?)