[AxPlugin] Events parameters are sent in the wrong order

VERIFIED FIXED

Status

()

Core
Embedding: APIs
VERIFIED FIXED
15 years ago
15 years ago

People

(Reporter: Adam Lock, Assigned: Adam Lock)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

884 bytes, patch
David Bradley
: review+
brendan
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

15 years ago
Events are fired with their parameters in the reverse order. The mistake crept
in during checkin of bug 178542.

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=XPConnect.cpp&root=/cvsroot&subdir=mozilla/embedding/browser/activex/src/plugin&command=DIFF_FRAMESET&rev1=1.17&rev2=1.18

Patch follows to correct the issue
(Assignee)

Comment 1

15 years ago
Created attachment 115015 [details] [diff] [review]
Patch

Simple patch grabs disp args in reverse order.

Proper ordering got lost in the churn of the event sink rewrite. The old
version got it right, the new one didn't.
Where are actual arguments being reversed?

/be
(Assignee)

Comment 3

15 years ago
In code like this:

<script for="someObject" event="FSCommand(command, args)">
</script>

Instead of (command, args) it is sent as (args, command) because the plugin code
that processes the args from the control is not putting them in the right order.

The reason is that IDispatch::Invoke sends its arguments in an array with the
args in reverse order, e.g.

pDispParams->rgvarg[0] = arg3;
pDispParams->rgvarg[1] = arg2;
pDispParams->rgvarg[2] = arg1;

The plugin used to have some code to put them in the proper order, but
unfortunately it was overlooked during a big code merge.

This small patch reinstates the correct behaviour.

Comment 4

15 years ago
Comment on attachment 115015 [details] [diff] [review]
Patch

r=dbradley
Attachment #115015 - Flags: review+
(Assignee)

Comment 5

15 years ago
Comment on attachment 115015 [details] [diff] [review]
Patch

Brendan, can you give this an sr? Thanks
Attachment #115015 - Flags: superreview?(brendan)
Comment on attachment 115015 [details] [diff] [review]
Patch

I was thrown by the comment, cuz it seemed to leave open the possibility that
the bizarre reason was not by design of IDispatch.  Maybe clarify it to say
"arguments are listed backwards, intentionally, in rgvarg." 
sr=brendan@mozilla.org in any event.

/be
Attachment #115015 - Flags: superreview?(brendan) → superreview+
(Assignee)

Comment 7

15 years ago
Thanks, I'll change the comment at checkin
(Assignee)

Comment 8

15 years ago
Comment on attachment 115015 [details] [diff] [review]
Patch

Requesting 1.3 apporval. ActiveX specific, low risk, obvious fix for
regression.
Attachment #115015 - Flags: approval1.3?

Comment 9

15 years ago
Comment on attachment 115015 [details] [diff] [review]
Patch

a=asa (on behalf of drivers) for checkin to 1.3
Attachment #115015 - Flags: approval1.3? → approval1.3+
(Assignee)

Comment 10

15 years ago
Fix is checked in
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Comment 11

15 years ago
[AxPlugin] -> Ashish
QA Contact: carosendahl → ashishbhatt

Comment 12

15 years ago
Verified on 2003-02-21
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.