Closed Bug 495618 Opened 15 years ago Closed 15 years ago

Reloading form posts fails with javascript nsIPromptService implementation

Categories

(Core :: DOM: Navigation, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mpgritti, Assigned: mpgritti)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
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?
Attachment #380618 - Attachment is obsolete: true
Attachment #381322 - Flags: superreview?(benjamin)
Attachment #381322 - Flags: review?(bzbarsky)
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 :)
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
Closed: 15 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.

Attachment

General

Created:
Updated:
Size: