608885 - Hitting the reload button on a page with POST results does nothing

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dave Miller [:justdave]]

When I'm on a page created as results of a POST, hitting the reload button does nothing.

My initial suspicion was the "are you sure?" dialog wasn't implemented yet, but mfinkle seems to think that should be there already on IRC.

Comment 1

[Mike Kristoffersen (:MikeK)]

It might be connected to the following error on stdout at the time when the dialog should be shown:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "browser is null" {file: "file:///home/mike/MozillaCode/101105/objdir-FFM_PD/dist/bin/components/PromptService.js" line: 218}]' when calling method: [nsIPrompt::confirmEx]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: chrome://browser/content/bindings/browser.js :: anonymous :: line 189"  data: yes]
************************************************************

Will try to figure out which context this is called in and why the browser is null...

Comment 2

[Mike Kristoffersen (:MikeK)]

Attachment: Forward call to confirmEx from content to parent process

Fits: 

changeset:   2349:757326bdbf5a
tag:         tip
user:        Vivien Nicolas <[e-mail]>
date:        Sat Nov 06 14:16:56 2010 +0100
summary:     Bug 603344: about:config add new integer pref UI is displaced to the left in portrait mode [r=mfinkle]

Comment 3

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

Comment on attachment 488917 [details] [diff] [review]
Forward call to confirmEx from content to parent process

I guess we should have expected this kind of error. We decided to fix them as we found them.

Comment 4

[Mike Kristoffersen (:MikeK)]

Thank you - It's great when things are reviewed before you get a chance to ask for the review :)

Is there a reason not just to do it for all the functions listed in "function PromptService() {" ?

const ALL_METHODS = ["alert", "alertCheck", "confirm", "prompt", "confirmEx", "confirmCheck", "select"];

Comment 5

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

We wanted to keep the exposed surface area to a minimum. We didn't want to open up all methods, just the ones we knew about.

I'm sure that will cause some bustage like this again. Not sure we still feel that proxying across is a worthy security risk or not.

Comment 6

[Matt Brubeck (:mbrubeck)]

(In reply to comment #5)
> We wanted to keep the exposed surface area to a minimum. We didn't want to open
> up all methods, just the ones we knew about.
> 
> I'm sure that will cause some bustage like this again. Not sure we still feel
> that proxying across is a worthy security risk or not.

But we already remote all of those functions if you call the PromptService methods directly.  It looks like this additional code is only necessary if you call them on the Prompt object returned by PromptService.getPrompt.

I don't understand what additional attack surface we'd expose by doing this in a generic wrapper around the Prompt object, instead of method-by-method.

Comment 7

[Matt Brubeck (:mbrubeck)]

Filed followup bug 610492 to do this in a more generic way.

Comment 8

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

Is this ready to land?

Comment 9

[Mike Kristoffersen (:MikeK)]

Yeah, was waiting for another mobile patch to piggy-bag it onto the try server with - will do today anyway.

Comment 10

[Mike Kristoffersen (:MikeK)]

Had some problems with the try server failing on all mobile builds today, but the error message didn't make sense so retried and it build'ed without any trouble the second time around - just asked for a run with the unit tests now.

In order to submit to the Mobile tree, does this patch need any approvals? - otherwise I'll push tomorrow morning (GMT) where I'll be able to watch the tree.

Comment 11

[Mike Kristoffersen (:MikeK)]

Pushed as:
http://hg.mozilla.org/mobile-browser/rev/7b56089c83d2

Comment 12

[Anna (Waverley)]

Can you provide a page created as results of a POST so that I can verify this bug?

Comment 13

[Dave Miller [:justdave]]

The original testcase was on Bugzilla, but Bugzilla handles form submissions differently than it used to, and we can't really duplicate the conditions via Bugzilla anymore.  It's basic form submission, should be easy to cobble up something probably.

Comment 14

[Dave Miller [:justdave]]

(I'm not planning to at the moment - busy with other stuff)

Comment 15

[Carla Nadastean]

Retested bug with:

Build id : Mozilla/5.0 (Android;Linux armv7l; rv:10.0a1)Gecko/20110928
Firefox/10.0a1 Fennec/10.0a1
Device: Motorola DROID 2
OS: Android 2.3

I visited a page that allows comments. to bring up the "Are you sure?" dialog( http://www.blogger.com/comment.g?blogID=199593421230317309&postID=5590131800074294106 )
Entered some text in the Comment text box and hit the reload button on device.

"Are you sure?" dialog with "Leave Page" and "Stay on page" options is displayed on device. 

Bug no longer reproducible, verifying bug.