As reported in https://github.com/mozilla-b2g/gaia/issues/4817 reload does not currently work for POST requests, presumably because the platform is waiting for confirmation from the user as in the desktop browser but no dialog is shown in the browser app UI. This is a good catch by Tim but a very late landing requirement, nominating for blocking-basecamp so Product can make a call but I'm not expecting this for 9/28. This is similar in nature to bug 775464 but at least this one doesn't cause a crash.

Justin, can you take this one?

I'm totally overloaded at the moment, but I'll see if I can find someone else to do this.

Patrick, are you interested in investigating this one? I don't know off the top of my head what's causing this problem. Off the top of my head, the ideal fix might be to make BrowserElementChild.reload() always do a reload, but to add a function to BEC which lets you check whether the current page has POST data (i.e., whether we should show the POST-refresh warning).

I reproduced this bug with browser element's mochitest (e.g. not on device). I think the nsDocShell does send the repost warning in nsDocShell::confirmRepost() [1]. It calls confirmEx to show the message. The message can be printed by adding a log in BrowserElementPrompt.confirmEx(). I think getting the message from confirmEx() might also fix this bug. [1] https://mxr.mozilla.org/mozilla-central/source/docshell/base/nsDocShell.cpp?rev=488beb32bca5#11067

Okay, we can fix it the right way! :)

Is this likely to be ready for today/tomorrow in time for feature complete? If you have some idea what the API will look like I can start working on the Gaia end, if necessary. Thanks

> Is this likely to be ready for today/tomorrow in time for feature complete? No. Even if someone coughed up a patch today, we can't even run a patch through tryserver in one day, much less review and land it. We shouldn't use the deadline to cause us to chase abnormally-fast fixes like this; that's only a distraction from the high-quality work we need to continue doing.

Patrick, you seem to understand what's going on here; can you take this bug?

Add an nsIConfirmRepostDetail interface and its implementation. This object is used to transfer the reference of window and the decision to repost between nsDocShell and BEC.

1. In nsDocShell::ConfirmRepost(), if nsDocShell found it is for a browser element, notify BEC through observer, and wait for the decision. 2. In BEC, handle the notification and show a sync prompt, with prompt type = "confirmrepost".

These patches are basically fine for what they're trying to do (modulo some English nits). I'm not wild about the way we call from DocShell into BEC, but I don't have a better suggestion offhand, so it's probably OK. But I'm concerned that we're working around this problem in the wrong way. We'll have this same problem anywhere we use ConfirmEx, right? If you search through the code, you'll see we use it in a number of places, for things such as * "Do you want to stay on this page?" triggered by an unload handler. * "This script is unresponsive; kill it?" * nsHttpChannelAuthProvider::ConfirmAuth, which I think happens when a page does something sketchy with http auth. I suspect we have bugs similar to this one lurking around each of these calls to ConfirmEx. So it seems to me that maybe the right thing to do is to pass an unlocalized identifier into ConfirmEx, modify all the ConfirmEx calls to include an identifier, and then handle the ConfirmEx call by passing that identifier to BEP and asking it to show an appropriate prompt. I have a few questions about this approach: 1) Would we be able to modify ConfirmEx to add an extra parameter, or would we need to create a new method and move all Gecko callers over to the new method? 2) Should we put the message identifiers into nsIPrompt.idl? It seems to me this would be a reasonable way of guaranteeing that there's a list somewhere of all the prompts we need to handle in BEC. An alternative approach would be to implement ConfirmEx in the browser-element code by passing the arguments straight through. That is, the browser element parent tells the embedder "do a prompt with these parameters", where the params come straight from ConfirmEx (maybe we modify them a bit to make a better interface, but they are basically unchanged). That probably wouldn't be too hard. My main concern with that approach are that: a) The embedder can't tell which prompt we're trying to show, because it only gets a localized message. So if, for example, the embedder wants to take a default action for a certain prompt, it can't. We could solve this by including a unique identifier with each prompt, as described above. b) We'd be passing localized params to the embedder, which we don't do elsewhere in the browser-element code -- that would make for a bad web-api. I don't know who owns the prompt code -- nsIPrompt.idl hasn't been touched for a while. bz, do you have any thoughts, or know who we should loop in here?

Comment on attachment 668386 [details] [diff] [review] Part 1: Add nsIConfirmRepostDetail to carry the message between nsDocShell and BEC Cancelling these reviews until we figure out how we want to proceed here.

bz, we could use some assistance here (comment 12), or a redirect to the right person.

Sorry, I had missed comment 12. bsmedberg may be a better person to ask about prompts....

<bsmedberg> jlebar: it looks to me like we shouldn't try to solve the "good API for prompts" problem for v1, so ISTM we should just pass the prompt args through (with whatever minimal security checking is necessary) That's the approach in comment 12 that starts with "An alternative approach ...". This is still a fair bit of work (particularly on the front-end), but at least at the end we'd have a correct implementation of confirmEx, which is better than we have now.

Comment on attachment 673172 [details] [diff] [review] Test case These tests are hard to write in a way that's readable, but I have some suggestions below. >+// Bug 793644, fire an event when attempting to reloads browser element after >+// POST respest. s/reloads/reload/ s/POST repest/a POST request/ >+"use strict"; >+SimpleTest.waitForExplicitFinish(); >+ >+var iframe; >+var gotConfirmRepost = false; >+var doRepost = true; >+var timer; >+ >+function getExpectedString() { >+ let result = {}; >+ let bundleService = SpecialPowers.Cc['@mozilla.org/intl/stringbundle;1']. >+ getService(SpecialPowers.Ci.nsIStringBundleService); >+ let appBundle = bundleService.createBundle("chrome://global/locale/appstrings.properties"); >+ let brandBundle = bundleService.createBundle("chrome://branding/locale/brand.properties"); >+ try { >+ let brandName = brandBundle.GetStringFromName("brandShortName"); >+ result.message = appBundle.formatStringFromName("confirmRepostPrompt", >+ [brandName], 1); >+ } catch (e) { >+ // for the case that we don't have brandShortName >+ result.message = appBundle.GetStringFromName("confirmRepostPrompt"); >+ } >+ result.resend = appBundle.GetStringFromName("resendButton.label"); >+ >+ return result; This function returns an object {message: "...", resend: "..."}, so it shouldn't be called "GetExpectedString". At least "GetExpectedStrings". >+function checkResult() { >+ ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload"); >+ if (doRepost) { >+ doRepost = false; >+ // Run again, with repost disallowed. >+ iframe.src = 'file_post_request.html'; >+ iframe.addEventListener('mozbrowserloadend', postPageLoadDone); >+ } else { >+ SimpleTest.finish(); >+ } >+} You're not really "checking a result" here; you're checking that we got a confirm-repost dialog and then running the next test if appropriate. I think it would be clearer if you inlined this function into its callers; both callers already branch on doRepost, so I don't think you'd end up repeating any code, and it would make the control-flow explicit. >+function reloadDone() { >+ if (!doRepost) { >+ // Browser element reloaded, we got a failure. >+ clearTimeout(timer); >+ timer = null; >+ ok(false, "Browser element reloaded when we don't allow it to reload."); >+ } >+ iframe.removeEventListener('mozbrowserloadend', reloadDone); >+ checkResult(); >+} >+ >+function postRequestDone() { >+ gotConfirmRepost = false; >+ iframe.removeEventListener('mozbrowserloadend', postRequestDone); >+ iframe.addEventListener('mozbrowserloadend', reloadDone); >+ if (!doRepost) { >+ // We don't expect browserelement really reload, use a timer to make sure >+ // it is not reloaded. s/really/to/ s/,/;/ >+ timer = window.setTimeout(checkResult, 1000); >+ } >+ iframe.reload(); >+} >+ >+function postPageLoadDone() { >+ iframe.removeEventListener('mozbrowserloadend', postPageLoadDone); >+ iframe.addEventListener('mozbrowserloadend', postRequestDone); >+} Rather than swapping out event listeners like this, I think it would be more clear if you had one event listener and you used a variable to determine its behavior. This is kind of hard to follow as-is. >+function runTest() { >+ browserElementTestHelpers.setEnabledPref(true); >+ browserElementTestHelpers.addPermission(); >+ >+ iframe = document.createElement('iframe'); >+ iframe.mozbrowser = true; >+ >+ iframe.src = 'file_post_request.html'; >+ document.body.appendChild(iframe); >+ >+ iframe.addEventListener('mozbrowserloadend', postPageLoadDone); >+ >+ let expectedMessage = getExpectedString(); >+ iframe.addEventListener('mozbrowsershowmodalprompt', function(e) { >+ if (e.detail.promptType == 'confirmEx') { >+ gotConfirmRepost = true; >+ e.preventDefault(); >+ e.detail.returnValue = { >+ button: doRepost ? 0 : 1, >+ }; >+ is(e.detail.button0.titleType, 'string'); >+ is(e.detail.button0.titleStr, expectedMessage.resend); >+ is(e.detail.message, expectedMessage.message); >+ is(e.detail.button1.titleType, 'no'); >+ e.detail.unblock(); Can we check that button2 is not specified? Also, I don't think titleStr should be defined if titleType is not 'string', so we should check that, too.

Comment on attachment 673171 [details] [diff] [review] Patch: Implement confirmEx function This is totally the right idea, but I'd prefer if we tweaked the interface we exposed to callers. >diff --git a/dom/browser-element/BrowserElementChild.js b/dom/browser-element/BrowserElementChild.js >--- a/dom/browser-element/BrowserElementChild.js >+++ b/dom/browser-element/BrowserElementChild.js >@@ -209,17 +209,18 @@ BrowserElementChild.prototype = { > > args.windowID = { outer: utils.outerWindowID, > inner: this._tryGetInnerWindowID(win) }; > sendAsyncMsg('showmodalprompt', args); > > let returnValue = this._waitForResult(win); > > if (args.promptType == 'prompt' || >- args.promptType == 'confirm') { >+ args.promptType == 'confirm' || >+ args.promptType == 'confirmEx') { Can we call this "custom-prompt" or something? >diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm > return this._browserElementChild.showModalPrompt( > this._win, {promptType: "confirm", title: title, message: text, returnValue: undefined}); > }, > > confirmCheck: function(title, text, checkMsg, checkState) { > return this.confirm(title, text); > }, > >+ // For each button, there is an object to descript its look Each button is described by an object with the following schema: ("descript" is not used in modern English; you mean "to describe".) >+ // { >+ // boolean show, // True if this button should be shown. >+ // string titleType, // 'ok', 'cancel', 'yes', 'no', 'save', 'dontsave', 'revert' or 'string' >+ // string titleStr, // Title in string, valid when titleType == 'string'. >+ // } >+ // Expected result from embedder: >+ // { >+ // int button, // Number of the button that user pressed: 0, 1, 2 >+ // boolean checked, // True if a check box is checked. >+ // } Can we modify the API sent to the embedder to make it a bit nicer? Instead of button0, button1, and button2, could we have a list of buttons? buttons: [button] button: {messageType: 'builtin', message: 'ok'/'cancel'/etc} and button: {messageType: 'custom', message: 'whatever string is passed'} I'd prefer if we also explicitly said whether we wanted to show the checkbox, as: showCheckbox: checkboxMessage: checkboxCheckedByDefault: true/false > confirmEx: function(title, text, buttonFlags, button0Title, button1Title, button2Title, checkMsg, checkState) { >- throw Cr.NS_ERROR_NOT_IMPLEMENTED; >+ let buttons = this._buildConfirmExButtonProperty(buttonFlags, button0Title, button1Title, button2Title); >+ let ret = this._browserElementChild.showModalPrompt( >+ this._win, >+ { >+ promptType: "confirmEx", >+ title: title, >+ message: text, >+ defaultButton: buttons.defaultButton, >+ button0: buttons.button0, >+ button1: buttons.button1, >+ button2: buttons.button2, >+ checkMsg: checkMsg, >+ checkState: checkState.value, >+ returnValue: undefined >+ } >+ ); >+ checkMsg = ret.checked; Do you mean |checkState.value = ret.checked|? >+ return ret.button; > }, >+ _buildConfirmExButtonProperty: function(buttonFlags, button0Title, button1Title, button2Title) { >+ let r = {}; >+ // Find the default button. >+ r.defaultButton = (buttonFlags & (3<<24)) >> 24; These magic numbers make me worried, mainly because we might change them in the nsIPrompt interface. I know it would be more verbose, but could you write this while assuming as little about the constants in the interface as possible? For example if (buttonFlags & Ci.nsIPrompt.BUTTON_POS_0_DEFAULT) {...} else if (...) {...} else if (...) {...} else { // throw an exception, or let button 0 be the default? } >+ // Properties of each button. >+ let titleTypeString = ['ok', 'cancel', 'yes', 'no', 'save', 'dontsave', 'revert', 'string']; >+ function buildButton(buttonFlags, buttonTitle, buttonNumber) { >+ let ret = {}; >+ let bitOffset = (buttonNumber * 8); I'm OK if you want to leave the * 8 here, because this is annoying to work around, but if you wanted to work around it, you could by doing something like iterating over the list [Ci.nsIPrompt.BUTTON_POS_0, Ci.nsIPrompt_BUTTON_POS_1, ...]. >+ let mask = Ci.nsIPrompt.BUTTON_TITLE_IS_STRING << bitOffset; >+ let titleType = (buttonFlags & mask) >> bitOffset; >+ if (titleType == 0) { >+ ret.show = false; >+ } else if (titleType < 127) { >+ ret.show = true; >+ ret.titleType = titleTypeString[titleType + 1]; >+ } else { // titleType == 127 >+ ret.show = true; >+ ret.titleType = 'string'; >+ ret.titleStr = buttonTitle; >+ } Please assert one way or another that your assumption that titleType <= 127 means that titleType == 127 is correct.

Summarize of this update: 1. Change name of prompt to 'custom-prompt'. 2. Use a list of buttons to pass the text on buttons to the embedder. 3. Use symbol in nsIPrompt instead of use the number directly. 4. Fix problems according to comment 20.

1. update test case with the change of patch. 2. According to comment 19, change some call flow and try to make it easier to read. Hi Justin, Because some of the flow in the test case is changed, would you help to review again? Thanks

Comment on attachment 674590 [details] [diff] [review] Patch: Implement confirmEx function v2 >diff --git a/dom/browser-element/BrowserElementPromptService.jsm b/dom/browser-element/BrowserElementPromptService.jsm >--- a/dom/browser-element/BrowserElementPromptService.jsm >+++ b/dom/browser-element/BrowserElementPromptService.jsm >@@ -46,18 +46,53 @@ BrowserElementPrompt.prototype = { > return this._browserElementChild.showModalPrompt( > this._win, {promptType: "confirm", title: title, message: text, returnValue: undefined}); > }, > > confirmCheck: function(title, text, checkMsg, checkState) { > return this.confirm(title, text); > }, > >- confirmEx: function(title, text, buttonFlags, button0Title, button1Title, button2Title, checkMsg, checkState) { >- throw Cr.NS_ERROR_NOT_IMPLEMENTED; >+ // Each button is described by an object with the following schema >+ // { >+ // string messageType, // 'builtin' or 'custom' >+ // string message, // 'ok', 'cancel', 'yes', 'no', 'save', 'dontsave', >+ // // 'revert' or a string from caller if messageType was 'custom'. >+ // } >+ // >+ // Expected result from embedder: >+ // { >+ // int button, // Index of the button that user pressed. >+ // boolean checked, // True if a check box is checked. >+ // } s/a check box/the checkbox/ -- the API only lets us have one checkbox. >+ confirmEx: function(title, text, buttonFlags, button0Title, button1Title, >+ button2Title, checkMsg, checkState) { >+ let buttons = this._buildConfirmExButtonProperty(buttonFlags, >+ button0Title, >+ button1Title, >+ button2Title); It's kind of misleading that we have a variable called "buttons" that isn't actually a list of buttons. Could you call this buttonProperties, maybe? Or alternatively, we could make use of a fancy language feature to do [buttons, defaultButton, indexToButtonNumberMap] = this._buildConfirmExButtonProperty() if _buildConfirmExButtonProperty() returned an array instead of an object. >+ let ret = this._browserElementChild.showModalPrompt( >+ this._win, >+ { >+ promptType: "custom-prompt", >+ title: title, >+ message: text, >+ defaultButton: buttons.defaultButton, >+ buttons: buttons.buttons, >+ showCheckbox: !!checkMsg, >+ checkboxMessage: checkMsg, >+ checkboxCheckedByDefault: !!checkState.value, >+ returnValue: { >+ button: buttons.defaultButton, Call this selectedButton? >+ checked: checkState.value >+ } >+ } >+ ); >+ checkState.value = ret.checked; >+ return buttons.mapIndexToButtonNumber[ret.button]; > }, >+ _buildConfirmExButtonProperty: function(buttonFlags, button0Title, >+ button1Title, button2Title) { _buildConfirmExButtonProperties might be a better name. >+ // If this is the default button, full the index of this button in >+ // array into r.defaultButton. This value is going to be exposed to >+ // embedder. >+ if (defaultButton === buttonNumber) { >+ r.defaultButton = r.buttons.length; >+ } "full" isn't a verb. "If this is the default button, set r.defaultButton to the index of this button in the array." perhaps? Also, this needs more articles: "in /the/ array", "to /the/ embedder". r=me with these nits fixed.

Comment on attachment 674599 [details] [diff] [review] Test case v2 This is a bit easier to follow; thanks. >+function pageLoadDone() { >+ if (!isPostRequestSubmitted) { >+ // This loadend is done by setting url in address bar, so we don't need to >+ // do anything. The test page will submit a POST request. >+ isPostRequestSubmitted = true; >+ return; >+ } >+ >+ gotConfirmRepost = false; >+ iframe.removeEventListener('mozbrowserloadend', pageLoadDone); >+ if (doRepost) { >+ iframe.addEventListener('mozbrowserloadend', reloadDone); >+ } else { >+ // We don't expect browserelement to reload; use a timer to make sure >+ // it is not reloaded. >+ iframe.addEventListener('mozbrowserloadend', failBecauseReloaded); >+ timer = window.setTimeout(function() { >+ iframe.removeEventListener('mozbrowserloadend', failBecauseReloaded); >+ ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload"); >+ SimpleTest.finish(); >+ }, 1000); This is asking for trouble (random oranges). In the last patch, as I understood it (and I may have misunderstood!), we were doing a SetTimeout() to make sure that something /didn't/ happen after one second. That's not ideal, but it's OK. But here, you're checking that the repost happens after one second, and that's not OK. >+ } >+ iframe.reload(); >+} >+ >+function runTest() { >+ browserElementTestHelpers.setEnabledPref(true); >+ browserElementTestHelpers.addPermission(); >+ >+ iframe = document.createElement('iframe'); >+ iframe.mozbrowser = true; >+ >+ isPostRequestSubmitted = false; >+ iframe.src = 'file_post_request.html'; >+ document.body.appendChild(iframe); >+ >+ iframe.addEventListener('mozbrowserloadend', pageLoadDone); >+ >+ let expectedMessage = getExpectedStrings(); >+ iframe.addEventListener('mozbrowsershowmodalprompt', function(e) { >+ if (e.detail.promptType == 'custom-prompt') { >+ gotConfirmRepost = true; >+ e.preventDefault(); >+ e.detail.returnValue = { >+ button: doRepost ? 0 : 1, >+ }; Hm. Optimally, the e.detail.returnValue that we pass in here wouldn't have the |checked| property unless !!checkMsg. But that's not a big deal; up to you if you want to fix it. >+ is(e.detail.buttons[0].messageType, 'custom'); >+ is(e.detail.buttons[0].message, expectedMessage.resend); >+ is(e.detail.buttons[1].messageType, 'builtin'); >+ is(e.detail.buttons[1].message, 'cancel'); Please check e.detail.buttons.length. >+ is(e.detail.message, expectedMessage.message); >+ is(e.detail.showCheckbox, false); >+ is(e.detail.checkMessage, null); >+ e.detail.unblock(); >+ } >+ }); >+} r- because we need to figure out the setTimeout issue.

(In reply to Justin Lebar [:jlebar] from comment #24) > >+ // We don't expect browserelement to reload; use a timer to make sure > >+ // it is not reloaded. > >+ iframe.addEventListener('mozbrowserloadend', failBecauseReloaded); > >+ timer = window.setTimeout(function() { > >+ iframe.removeEventListener('mozbrowserloadend', failBecauseReloaded); > >+ ok(gotConfirmRepost, "Didn't get confirmEx prompt before reload"); > >+ SimpleTest.finish(); > >+ }, 1000); > > This is asking for trouble (random oranges). > > In the last patch, as I understood it (and I may have misunderstood!), we > were > doing a SetTimeout() to make sure that something /didn't/ happen after one > second. That's not ideal, but it's OK. But here, you're checking that the > repost happens after one second, and that's not OK. I use setTimeout() here for the same reason as setTimeout() in previous version: make sure 'mozbrowserloadend' isn't fired in a second. If 'mozbrowserloadend' is called before time out, the test will fail. But if 'mozbrowserloadend' isn't called before time out, we might still need to check if 'mozbrowsershowmodalprompt' has been fired, so I put a test in the time out handler. I think I should move the 'setTimeout' into the handler of 'mozbrowsershowmodalprompt', otherwise, if that event isn't fired in a second, the test will fail.

> I think I should move the 'setTimeout' into the handler of > 'mozbrowsershowmodalprompt', otherwise, if that event isn't fired in a > second, the test will fail. I think I understand, and what you're saying sounds right.

Fix previous patch according to comment 23 and comment 24.

1. Fix according to comment 24. 2. Move setTimeout() to handler of mozbrowsershowmodalprompt.

Comment on attachment 675631 [details] [diff] [review] Test case v3 Yes, this looks good to me.

Let me know when you're ready to check this in.

Try: https://tbpl.mozilla.org/?tree=Try&rev=ce356a7162cf

https://hg.mozilla.org/integration/mozilla-inbound/rev/698b72833c2b https://hg.mozilla.org/integration/mozilla-inbound/rev/085868531b69

https://hg.mozilla.org/mozilla-central/rev/698b72833c2b https://hg.mozilla.org/mozilla-central/rev/085868531b69

https://hg.mozilla.org/releases/mozilla-aurora/rev/795246ea4762 https://hg.mozilla.org/releases/mozilla-aurora/rev/f93e73154ea7