calling unused methods would break OS/2 d&d

RESOLVED FIXED

Status

()

Core
Drag and Drop
--
minor
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: Rich Walsh, Assigned: mkaply)

Tracking

Trunk
x86
OS/2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

13 years ago
The OS/2 version of nsDragService doesn't use the methods StartDragSession() or
EndDragSession() which are inherited from nsBaseDragService.  If some code were
to call them (in error), they would break our implementation.  This patch
overrides these two methods and turns them into no-ops to prevent this possibility.
(Reporter)

Comment 1

13 years ago
Created attachment 178330 [details] [diff] [review]
patch to nsDragService for OS/2

This patch should be applied to the trunk and all applicable branches.

Comment 2

13 years ago
This is a security measure against malitious extensions following bug 285438? Or
does it have implications that I don't see because of which I should refresh my
unofficial 1.7.6?
As a nit, perhaps you could return NS_ERROR_NOT_IMPLEMENTED instead of NS_OK?
(Reporter)

Comment 3

13 years ago
(In reply to comment #2)
> This is a security measure against malitious extensions following bug 285438?
> Or does it have implications that I don't see because of which I should
> refresh my unofficial 1.7.6?

Not really.  This is just protection against future additions to the codebase
doing something stupid.  The two methods are declared as public, so anyone
anywhere could call them in error and bolix up OS/2 d&d.  Had they been declared
as protected, so that only subclasses of nsBaseDragService could call them, then
this wouldn't be needed.

WRT the security bug, the OS/2 version of nsDragService had _no_ problems.

> As a nit, perhaps you could return NS_ERROR_NOT_IMPLEMENTED instead of NS_OK?

Returning an error might break stupid code and give the mistaken impression that
our code was at fault.  IMHO, it's better to let whatever proceed in blissful
ignorance, generating an assertion in the debug version but otherwise doing no harm.

Comment 4

13 years ago
Comment on attachment 178330 [details] [diff] [review]
patch to nsDragService for OS/2

Fine, you've convinced me. I hope I am not crossing the line by marking r=me.
Attachment #178330 - Flags: superreview?(mkaply)
Attachment #178330 - Flags: review+
(Assignee)

Comment 5

13 years ago
Comment on attachment 178330 [details] [diff] [review]
patch to nsDragService for OS/2

sr=mkaply
Attachment #178330 - Flags: superreview?(mkaply) → superreview+
(Assignee)

Comment 6

13 years ago
I put this on trunk. It's getting harder to get non security stuff on the
branch. If you can come up with a really good reason...
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
(Reporter)

Comment 7

13 years ago
(In reply to comment #6)
> I put this on trunk. It's getting harder to get non security stuff on the
> branch. If you can come up with a really good reason...

This is simply insurance against poorly written new features.  If there are no
new features added to a branch, no insurance is needed.
You need to log in before you can comment on or make changes to this bug.