791372 - Navigator::MozIsLocallyAvailable doesn't associate its docshell with a load group

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[(no longer active)]

http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#885

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 661369 [details] [diff] [review]
Patch (v1)

>Bug 791372 - Navigator::MozIsLocallyAvailable doesn't associate its docshell with a load group; r=bzbarsky

s/docshell/channel/ ?

>+++ b/dom/base/Navigator.cpp
>+  if (win) {

Shouldn't we just return false or something and not even do the network request if !win?  Otherwise it seems like holding on to a navigator object from a closed window will let people call into necko here with a null loadgroup.

That said, I'm not sure why this change matters.  This is a sync load guaranteed to not hit the network.  Why does it need to be in a loadgroup?

Comment 3

[(no longer active)]

(In reply to Boris Zbarsky (:bz) from comment #2)
> Comment on attachment 661369 [details] [diff] [review]
> Patch (v1)
> 
> >Bug 791372 - Navigator::MozIsLocallyAvailable doesn't associate its docshell with a load group; r=bzbarsky
> 
> s/docshell/channel/ ?

Heh, good point.

> >+++ b/dom/base/Navigator.cpp
> >+  if (win) {
> 
> Shouldn't we just return false or something and not even do the network
> request if !win?  Otherwise it seems like holding on to a navigator object
> from a closed window will let people call into necko here with a null
> loadgroup.

Sure, sounds good.

> That said, I'm not sure why this change matters.  This is a sync load
> guaranteed to not hit the network.  Why does it need to be in a loadgroup?

Good point.  I'm interested in doing this here even though it doesn't matter in practice right now since in the future we might use this information for loads that don't hit the network too, so it's best to make sure we're covering all grounds right now.

Comment 4

[(no longer active)]

Attachment: Patch (v2)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 661864 [details] [diff] [review]
Patch (v2)

OK, r=me with the previous caveats.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Er, you already applied the caveats.  Just r=me.  ;)

Comment 7

[(no longer active)]

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

Comment 8

[Graeme McCutcheon [:graememcc]]

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