Closed Bug 391978 Opened 14 years ago Closed 14 years ago

[FIX]nsExternalProtocolHandler leak

Categories

(Core :: Networking, defect, P1)

x86
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta2

People

(Reporter: sayrer, Assigned: bzbarsky)

References

Details

(Whiteboard: [has-patch])

Attachments

(3 files)

Attached file stack
reproduce by loading layout/reftests/bugs/374193-1.xhtml
So... What's going on here is that the patch that landed for bug 237312 doesn't match the patch that got reviews in the bug.  Compare what landed:

 http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/netwerk/base/src&command=DIFF_FRAMESET&file=nsIOService.cpp&rev2=1.175&rev1=1.174

to what got reviews:  https://bugzilla.mozilla.org/attachment.cgi?id=143754&action=diff

I'm guessing there were merge issues, since 5 months passed between reviews and landing and bug 244220 landed during that time.

In particular, as landed, the patch broke cases when |externalProtocol| was set to true to just ignore that setting.  Then we proceeded to fix that situation in a totally different way in bug 263546 and bug 173010.  I'm going to just remove the code that hasn't ever worked, except for about three months in 2004 (between bug 244220 landing and bug 237312 landing).
Component: Layout → Networking
QA Contact: layout → networking
Attached patch Proposed fixSplinter Review
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attached patch Same as diff -wSplinter Review
In particular this actually makes gnome-vfs follow the pref like it was supposed to....

dmose, you've been in this code (exthandler) most recently; can you double-check that we really don't need the blocked protocol handler thing?
Attachment #276435 - Attachment is obsolete: true
Attachment #276437 - Flags: superreview?(dmose)
Attachment #276437 - Flags: review?(cbiesinger)
Priority: -- → P1
Summary: nsExternalProtocolHandler leak → [FIX]nsExternalProtocolHandler leak
Target Milestone: --- → mozilla1.9 M8
Comment on attachment 276437 [details] [diff] [review]
Same as diff -w

listedProtocol is unused now in the nsIOService.cpp part of the patch
Attachment #276437 - Flags: superreview?(dmose) → superreview+
Comment on attachment 276437 [details] [diff] [review]
Same as diff -w

oops, wrong flag
Attachment #276437 - Flags: superreview?(dmose)
Attachment #276437 - Flags: superreview+
Attachment #276437 - Flags: review?(cbiesinger)
Attachment #276437 - Flags: review+
Attachment #276437 - Flags: superreview?(dmose)
Attachment #276437 - Flags: superreview?(dmose)
It looks to me as though this patch will effectively change Gnome VFS from being considered "external" to being considered "internal".  Eg smb: via Gnome VFS will be allowed with this patch, even if the pref is set to forbid an external handler for smb:.  This doesn't seem like a great idea to me.  But maybe I'm misunderstanding the code?

In general, the implmentation of these preferences is spread out in a way that makes it pretty hard for me to be confident in changes here.  I think it would be hugely helpful to have a bunch of tests for both nsIIOService.getProtocolHandler and nsIExternalProtocolService.loadURI for the various different combinations of internal, external, and GnomeVFS handlers (assuming we want to keep GNOME VFS handlers) along with the various different pref values.

Thoughts?
Attachment #276435 - Attachment is obsolete: false
> It looks to me as though this patch will effectively change Gnome VFS from
> being considered "external" to being considered "internal"

In the current code they already sorta are (though perhaps this is a mistake).  And frankly they _are_ "internal" in some ways: they behave just like HTTP does in terms of rendering in the content area, not in a helper app.

In the current code (without my patch), there are three possible states for the "external" pref: true, false, and unset.  Right now the behavior for GNOME-vfs stuff is:

unset: go ahead and do gnome-vfs stuff
false: Leak and then go ahead and do gnome-vfs stuff
true: go ahead and do gnome-vfs stuff

I went ahead and fixed the leak.  I guess we could instead change things so that we'll skip gnome-vfs stuff if explicitly listed as not external.  Would you prefer that?

> In general, the implmentation of these preferences is spread out

Yes, it is.  See comment 1 about the checkin history.  :(

And yes, it would be great to write those tests.  I'm not going to be in a position to do that until spring, though, now that classes have started....
I really think we should get this fixed for 1.9... waiting on reviewer feedback here.
Flags: blocking1.9?
Target Milestone: mozilla1.9 M8 → mozilla1.9 M10
Flags: blocking1.9? → blocking1.9+
Whiteboard: [has-patch]
(In reply to comment #7)
> In the current code they already sorta are (though perhaps this is a mistake). 
> And frankly they _are_ "internal" in some ways: they behave just like HTTP does
> in terms of rendering in the content area, not in a helper app.

You're right that the GNOME-vfs things are in an odd hybrid sort of place.

As I understand it, the point of the "external" pref is not to characterize how a specific handler is treated by the gecko code, but to assert the amount of trust that should be placed in it based in part on whether it was designed to be used in this specific context.

> In the current code (without my patch), there are three possible states for the
> "external" pref: true, false, and unset.  Right now the behavior for GNOME-vfs
> stuff is:
> 
> unset: go ahead and do gnome-vfs stuff
> false: Leak and then go ahead and do gnome-vfs stuff
> true: go ahead and do gnome-vfs stuff
> 
> I went ahead and fixed the leak. 

You're right; I misread the code flow.

> I guess we could instead change things so
> that we'll skip gnome-vfs stuff if explicitly listed as not external.  Would
> you prefer that?

I think we should do that.  However, whether you want to do that as part of this bug is up to you.  If not, can you spin off another bug?

sr=dmose; sorry for the delay.
Attachment #276437 - Flags: superreview?(dmose) → superreview+
Comment on attachment 276437 [details] [diff] [review]
Same as diff -w

bz, please add this to your checkin queue. thanks.
Attachment #276437 - Flags: approval1.9+
Checked in.  Filed bug 403161 to figure out how we really want to handle gnome-vfs here.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Depends on: 661967
You need to log in before you can comment on or make changes to this bug.