ParamTraits<nsQueryContentEvent>::Read is wrong

RESOLVED FIXED in mozilla5

Status

()

defect
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: jdm, Assigned: smichaud)

Tracking

unspecified
mozilla5
x86
Linux
Points:
---

Firefox Tracking Flags

(blocking2.0 .x+)

Details

Attachments

(1 attachment)

(Reporter)

Description

8 years ago
../../dist/include/IPC/nsGUIEventIPC.h: In static member function ‘static bool IPC::ParamTraits<nsQueryContentEvent>::Read(const IPC::Message*, void**, IPC::ParamTraits<nsQueryContentEvent>::paramType*)’:
../../dist/include/IPC/nsGUIEventIPC.h:258: warning: ignoring return value of ‘bool IPC::ReadParam(const IPC::Message*, void**, P*) [with P = PRUint8]’, declared with attribute warn_unused_result
PContentParent.cpp

>    return ReadParam(aMsg, aIter, static_cast<nsGUIEvent*>(aResult)) &&
>           ReadParam(aMsg, aIter, &aResult->mSucceeded) &&
>           ReadParam(aMsg, aIter, &aResult->mInput.mOffset) &&
>           ReadParam(aMsg, aIter, &aResult->mInput.mLength) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mOffset) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mString) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mRect) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mReversed) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mHasSelection);
>           ReadParam(aMsg, aIter, &aResult->mReply.mWidgetIsHit);

The original bug that regressed this was for mac-only, so this may not actually need to block Fennec.  Still, this is badness.
Looks like I made a typo.

Presumably this:

> ...
>           ReadParam(aMsg, aIter, &aResult->mReply.mHasSelection);
>           ReadParam(aMsg, aIter, &aResult->mReply.mWidgetIsHit);

should be changed to this:

> ...
>           ReadParam(aMsg, aIter, &aResult->mReply.mHasSelection) &&
>           ReadParam(aMsg, aIter, &aResult->mReply.mWidgetIsHit);

Sorry for the dumb mistake :-(
I'm not sure what the consequences of this mistake are for 2.0 -- I don't believe mWidgetIsHit is currently used from IPC code.

In any case, it's a very simple fix.
Posted patch FixSplinter Review
Assignee: nobody → smichaud
Status: NEW → ASSIGNED
Attachment #517169 - Flags: review?(benjamin)
I'm marking this blocking2.0? to bring it to peoples' attention.

I don't believe my original mistake has any consequences for 2.0 (aside from triggering nasty compiler warnings).  But it's extremely easy to fix, with zero risk.
blocking2.0: --- → ?
This seems like an ideal candidate for an "rc ridealong" (http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/7426ebdb1338dd40#).

Updated

8 years ago
Attachment #517169 - Flags: review?(benjamin) → review+
Attachment #517169 - Flags: approval2.0?

Comment 6

8 years ago
Not a blocker.
blocking2.0: ? → -
tracking-fennec: ? → ---

Updated

8 years ago
Attachment #517169 - Flags: approval2.0?
Depends on: post2.0
OS: Linux → Windows CE
OS: Windows CE → Linux
http://hg.mozilla.org/mozilla-central/rev/60e1f37ed719
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
No longer depends on: post2.0
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2

Updated

8 years ago
blocking2.0: - → .x+

Updated

8 years ago
Attachment #517169 - Flags: approval2.0?
What's the rationale for the approval request? looks like it was removed at least once already. It does seem like a safe fix, but does it actually make a difference enough to be worth the QA brainprint?
> looks like it was removed at least once already.

Where on earth did you get this idea? :-)

> It does seem like a safe fix, but does it actually make a difference
> enough to be worth the QA brainprint?

It's a trivial, no-risk patch.  It fixes a typo.  And while the typo
currently doesn't break anything, it does cause a nasty compile error.

Updated

8 years ago
Attachment #517169 - Flags: approval2.0?
You need to log in before you can comment on or make changes to this bug.