Reloading form posts fails with javascript nsIPromptService implementation

RESOLVED FIXED

Status

()

Core
Document Navigation
RESOLVED FIXED
9 years ago
4 years ago

People

(Reporter: Marco Pesenti Gritti, Assigned: Marco Pesenti Gritti)

Tracking

unspecified
x86
Mac OS X
Points:
---
Bug Flags:
blocking1.9.1.1 -
wanted1.9.1.x +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

9 years ago
We have a xulrunner application which provides a custom nsIPromptService implementation in javascript. It mostly works but when trying to reload a post form no dialog is displayed and we get a NS_ERROR_ILLEGAL_VALUE error.

This happens because nsDocShell::ConfirmRepost calls prompter->ConfirmEx passing nsnull as checkState. It looks like xpconnect rejects null values for out paramaters.
(Assignee)

Comment 1

9 years ago
Created attachment 380618 [details] [diff] [review]
Pass a checkState even if we don't use it

There are probably other places where we are calling confirmEx and other nsIPromptService methods passing nsnull to out parameters, I can write a patch to fix those too if this approach is fine.

I couldn't find anything in the xpconnect documentation about passing null as out parameter, but looking at the code it seems like they are refused intentionally. Suggestions about better ways of fixing the issue are welcome.
Attachment #380618 - Flags: review?
(Assignee)

Comment 2

9 years ago
Here is where the call is aborted because of the null parameter:
http://mxr.mozilla.org/mozilla-central/source/js/src/xpconnect/src/xpcwrappedjsclass.cpp#1463
Comment on attachment 380618 [details] [diff] [review]
Pass a checkState even if we don't use it

You really want to request review from a particular person, so the review request doesn't get lost...  If you're not sure whom to ask, check in #developers on irc.mozilla.org.  ;)

That said, this looks great; passing null for an inout param is in fact an API violation...

If you do want to look for other instances of such calls to ConfirmEx, that would be great; once we eliminate them, I think we should remove the null-check in ConfirmEx and replace it with either a fail-on-null or just an assertion.  That should be a separate bug, though.

Do you need this landed on the 1.9.1 branch as well as trunk?  I'll land on trunk as soon as we reopen it (hopefully a few days at most).
Attachment #380618 - Flags: review? → review+
(In reply to comment #3)
> (From update of attachment 380618 [details] [diff] [review])

> Do you need this landed on the 1.9.1 branch as well as trunk?  I'll land on
> trunk as soon as we reopen it (hopefully a few days at most).

Fennec has plans to reimplement nsIPromptService in JS for it's 1.0 release. That won't be 1.9.1, but would be 1.9.1.x

I'm not sure it wanted-1.9.1 is good enough for that.
tracking-fennec: --- → ?
Flags: wanted1.9.1?
(Assignee)

Comment 5

9 years ago
Created attachment 381322 [details] [diff] [review]
Always pass a checkState to nsIPromptService methods even if we don't use it
Attachment #380618 - Attachment is obsolete: true
Attachment #381322 - Flags: superreview?(benjamin)
Attachment #381322 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

9 years ago
The updated patch should fix all the nsIPromptService calls, I hope I've not missed any.

Sorry about the missing review person. I thought I had typed on in, maybe I typoed it or something :)
(Assignee)

Comment 7

9 years ago
Landing in 1.9.1 would be good if possible.
Attachment #381322 - Flags: superreview?(benjamin)
Attachment #381322 - Flags: superreview+
Attachment #381322 - Flags: review?(bzbarsky)
Attachment #381322 - Flags: review+
Assignee: nobody → mpgritti
Flags: wanted1.9.1.x?
Whiteboard: [3.5.1?]
Pushed http://hg.mozilla.org/mozilla-central/rev/7fd31bcba0fc

Nominated for 1.9.1.x.

Thanks for the patch!
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Flags: blocking1.9.1.1?
Whiteboard: [3.5.1?]
Not blocking 1.9.1.1, but we'll consider a patch for 1.9.1.2 or later.
Flags: wanted1.9.1.x?
Flags: wanted1.9.1.x+
Flags: blocking1.9.1.1?
Flags: blocking1.9.1.1-
Flags: wanted1.9.1?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.