495618 - Reloading form posts fails with javascript nsIPromptService implementation

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Marco Pesenti Gritti]

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.

Comment 1

[Marco Pesenti Gritti]

Attachment: 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.

Comment 2

[Marco Pesenti Gritti]

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 3

[Boris Zbarsky [:bzbarsky]]

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).

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(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.

Comment 5

[Marco Pesenti Gritti]

Attachment: Always pass a checkState to nsIPromptService methods even if we don't use it

Comment 6

[Marco Pesenti Gritti]

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 :)

Comment 7

[Marco Pesenti Gritti]

Landing in 1.9.1 would be good if possible.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/7fd31bcba0fc

Nominated for 1.9.1.x.

Thanks for the patch!

Comment 9

[Samuel Sidler (old account; do not CC)]

Not blocking 1.9.1.1, but we'll consider a patch for 1.9.1.2 or later.