STR: 1. Create an app that has a message handler on a different URI than the normal launch one. For example: Normal launch: index.html Handler URL: handler.html 2. Load the handler.html on a iframe from the index.html. 3. Load the app and send it to the background 4. Send the message that the app is waiting for Expected: The message is processed correctly. The app shown page isn't changed. Actual: The message is processed... or not. The synchronous part is processed, but any asynchronous process won't happen. What's happening is: 1. The parent process sees that there's already a window/frame that has the system message processing URI (handler.html). 2. The parent process sends a SystemMessageManager:Message message to the process running the app. 3. The child process tells the parent process that it got the message and calls the handler. Before even doing that, it tells the parent that it got the message. Ad this removes it from the parent queue of pending messages. 3' (3, on the parent): The parent process sends a system-messages-open-app. 4'. The system app (on the parent process) checks that the loaded URL (index.html) at the app main frame isn't the same than the processing URI (handler.html). And changes the src 5. At this point, the handler code is interrupted (usually after the synchronous part has finished running) and the handler.html page is loaded. But the message ISN'T processed again (or correctly, or at all) because at step 3 the message was marked as processed. What should be done here is that the system-messages-open-app should be sent *only* when the parent process hasn't already found a window to deliver the message (and hasn't done it so already). Cause otherwise we end loading the handler page when the message has already been processed (in the best case) or we interrupt the processing (at the worst case).

Requesting leo+ since this is a blocker of a blocker

You can find such an application here: https://github.com/lodr/gaia/tree/lost-messages

Why it is designed in this way in the first place is because we want to make sure the app must be launched in any way to handle the system message. - If the app is not yet launched, 'system-messages-open-app' will take effect to launch the app to retrieve the pending messages. - If the app is launched, the message can be directly delivered through IPC. We'll still fire 'system-messages-open-app' but it shouldn't take effect in the window_manager.js because it must know the app is already running. We fire 'system-messages-open-app' also for case 2 to avoid any chances of race conditions. You can imagine what's happening if the user or OOM kill the running app before it successfully handles the coming system messages. We have no way to wake up the app anymore if we don't fire 'system-messages-open-app' for that. Also, 'system-messages-open-app' carries the page URI. Sometimes, we want to pull a certain page (within the same app) to the foreground to handle the system message, so we fire 'system-messages-open-app' again for switching the app to the specific page even if the app is still running. Looping more people that used to be involved in the System Message API.

(In reply to Gene Lian [:gene] from comment #3) > Why it is designed in this way in the first place is because we want to make > sure the app must be launched in any way to handle the system message. > > - If the app is not yet launched, 'system-messages-open-app' will take > effect to launch the app to retrieve the pending messages. > > - If the app is launched, the message can be directly delivered through > IPC. We'll still fire 'system-messages-open-app' but it shouldn't take > effect in the window_manager.js because it must know the app is already > running. > > We fire 'system-messages-open-app' also for case 2 to avoid any chances of > race conditions. You can imagine what's happening if the user or OOM kill > the running app before it successfully handles the coming system messages. > We have no way to wake up the app anymore if we don't fire > 'system-messages-open-app' for that. The thing is... the way it's implemented, the situation you're described won't work correctly either, because the message is acknowledged (via a SystemMessageManager:Message:Return:OK) before the handler is even invoked. That means that the message will be deleted... and even if we wake up the app again, the message won't be delivered again. And, in fact, the comment at [1] specifically says that the message will be removed so as to avoid 'the relaunched app handling it again' If that was the intention, what should be done here is to delay sending the Return:OK until the message has actually been processed. But that would make invalid the comment at [1] :) > Also, 'system-messages-open-app' carries the page URI. Sometimes, we want to > pull a certain page (within the same app) to the foreground to handle the > system message, so we fire 'system-messages-open-app' again for switching > the app to the specific page even if the app is still running. Hmm but the message is delivered to the child process only if the parent process knows that the correct URI is loaded on a window at the child process. And then the message is delivered to that window. That's currently working correctly (if you have the handler URI loaded on an iframe it's invoked). [1] https://mxr.mozilla.org/mozilla-b2g18/source/dom/messages/SystemMessageInternal.js#420

(In reply to Antonio Manuel Amaya Calvo from comment #4) > The thing is... the way it's implemented, the situation you're described > won't work correctly either, because the message is acknowledged (via a > SystemMessageManager:Message:Return:OK) before the handler is even invoked. > That means that the message will be deleted... and even if we wake up the > app again, the message won't be delivered again. > > And, in fact, the comment at [1] specifically says that the message will be > removed so as to avoid 'the relaunched app handling it again' > > If that was the intention, what should be done here is to delay sending the > Return:OK until the message has actually been processed. But that would make > invalid the comment at [1] :) Nice catch. You're pointing out another potential defect in the original design. As you mentioned at bug 888609, comment #9, it's too early to cancel the pending messages via "SystemMessageManager:Message:Return:OK". Btw, if we really want to fix that, I'd suggest let's rename this message to be something like: "SystemMessageManager:HandlingMessages" > Hmm but the message is delivered to the child process only if the parent > process knows that the correct URI is loaded on a window at the child > process. And then the message is delivered to that window. That's currently > working correctly (if you have the handler URI loaded on an iframe it's > invoked). I'm talking about the window_manager.js at [1]. We somehow need 'system-messages-open-app' to switch the app to the specific page even if the app is already running. [1] https://github.com/mozilla-b2g/gaia/blob/10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.js#L1330

(In reply to Gene Lian [:gene] from comment #5) > I'm talking about the window_manager.js at [1]. We somehow need > 'system-messages-open-app' to switch the app to the specific page even if > the app is already running. Of course but, what if the **specific page** is already running (as an iframe inside the main UI)? That's the problem we have in Cost Control. Look at the comment in L1328 and L1329: > // If the app is opened and it is loaded to the correct page, > // then there is nothing to do. The correct comment should be: > // If the app is opened and it is loaded to the correct page > + // or has an iframe with the correct page, > // then there is nothing to do.

The problem isn't window_manager.js, or it should not be fixed there. The problem is that gecko does already know if the handler page is loaded or not (and it uses that information to send the message to the correct process/window in fact), but still sends the open-app message even when it has in fact already invoked the handler. What I think should be done here is... ah, it's easier if I just make a WIP patch to show it :)

This is a possible solution (untested, just to show what I think the problem is)

Oh, in case we agree this approach is good for a solution, I can take the bug and finish the patch.

Taking this. Unless you have something against the approach I described on the patch I'll finish it that way.

(In reply to Salvador de la Puente González [:salva] from comment #6) > (In reply to Gene Lian [:gene] from comment #5) > > I'm talking about the window_manager.js at [1]. We somehow need > > 'system-messages-open-app' to switch the app to the specific page even if > > the app is already running. > > Of course but, what if the **specific page** is already running (as an > iframe inside the main UI)? That's the problem we have in Cost Control. Look > at the comment in L1328 and L1329: > > > // If the app is opened and it is loaded to the correct page, > > // then there is nothing to do. > > The correct comment should be: > > > // If the app is opened and it is loaded to the correct page > > + // or has an iframe with the correct page, > > // then there is nothing to do. Please look at [1]. It's not actually an arbitrary opened iframe but the one of |.firstChild|. I'll ping Fabrice and Alive to confirm this. Please stay tuned. [1] https://github.com/mozilla-b2g/gaia/blob/10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.js#L1326

Hi Alive and Fabrice, In short, the root cause of this bug is the platform is firing 'open-app' to open the page again even if the page is already running. I have a big concern at [1], where the 'open-app' chrome event carries the page URL to let window_manager.js decide if it needs to reset the iframe.src to that page. I'm worrying about if we don't fire 'open-app' for this purpose, we'd have other bad regressions, though. If we still need some mechanism to ask the app to switch to a certain page, maybe we should fire a separate event like 'load-page' which is only in charge of switching the app to the desired page, instead of opening the whole app by 'open-app'. What do you think? [1] https://github.com/mozilla-b2g/gaia/blob/10c78c2a4345e71a52f29ea67ad2a6b7d0dc6c13/apps/system/js/window_manager.js#L1326

(In reply to Antonio Manuel Amaya Calvo from comment #10) > Taking this. Unless you have something against the approach I described on > the patch I'll finish it that way. The patch basically looks good to me. However, we need to confirm comment #12 first to avoid regression. It's a bit dangerous to have this change during the 1.1 cycle. We need to be careful. :)

Hi, My question is: if you embed handler.html in index.html, that is to say, you have to load the handler.html at first time index.html loaded. System app couldn't load your own iframe in your app. We cannot touch content of you app. So that, I think you have to specify the message handler page is "index.html" instead of "handler.html" in manifest. "messages": [ { "alarm": "/index.html" } ] Because exactly you put setMessageHandler under index.html's iframe, not 'handler.html' itself. Does this resolve your problem? I do think system message is not perfect now but I don't think we should accept this patch. Or else we will have some 'real' handler app opened and alive but couldn't receive system message because we don't change the src to real handler page because open-app event is blocked by gecko. Briefly, my proposal is write down 'Real' system message handler "top" page location in manifest instead the page that you embedded in your top page.

To answer Alive's question, if your app only has one HTML, or if the system message handler is defined to always be the top level HTML, the system message works correctly. But it does so because in that case, if the app is already running, the open-app message sent to the system app in Gaia is just a no-op: it doesn't do anything except wasting a few CPU cycles. If the system message handler is embedded on another HTML, though, the current code doesn't work at all, because first the message is sent (and acknowledged) and then the handler page is loaded. That load is both unnecessary (since the message has already been delivered to the correct frame) and incorrect (since it breaks the desired/expected behavior). The proposed patch just fixes that. Note that, regarding regressions, this patch won't affect at all existing apps that define the handler at the top level window. For those apps this patch will only make the code slightly more efficient. And without the patch, any app that doesn't have the message handler at the top window just doesn't work correctly.

Hello people. (In reply to Alive Kuo [:alive] from comment #14) > Hi, > > My question is: if you embed handler.html in index.html, > that is to say, you have to load the handler.html at first time index.html > loaded. > > System app couldn't load your own iframe in your app. We cannot touch > content of you app. > > So that, I think you have to specify the message handler page is > "index.html" instead of "handler.html" in manifest. You can do so, but it does not solve the problem in the BE. The behavior is still incorrect. Anyway, take in count this use case: suppose you have a lightweight message handler which need to receive alarms (only received by the webpage which installs the alarm), process, then close. If you want to handle the alarms when the application is open, you need to include the origin inside the application. Hope it gives you a clearer point of view. :) FYI: Including handling inside index.html is another proposal already rejected twice: bug 834334

(In reply to Antonio Manuel Amaya Calvo from comment #15) > To answer Alive's question, if your app only has one HTML, or if the system > message handler is defined to always be the top level HTML, the system > message works correctly. But it does so because in that case, if the app is > already running, the open-app message sent to the system app in Gaia is just > a no-op: it doesn't do anything except wasting a few CPU cycles. > > If the system message handler is embedded on another HTML, though, the > current code doesn't work at all, because first the message is sent (and > acknowledged) and then the handler page is loaded. That load is both > unnecessary (since the message has already been delivered to the correct > frame) and incorrect (since it breaks the desired/expected behavior). > > The proposed patch just fixes that. Note that, regarding regressions, this > patch won't affect at all existing apps that define the handler at the top > level window. For those apps this patch will only make the code slightly > more efficient. And without the patch, any app that doesn't have the message > handler at the top window just doesn't work correctly. I have no strong opinion if this change doesn't break current any system message handler. BTW, I think we don't have a clear spec about how app should deal with system message. Think about this case: (1) index.html is for UI rendering (2) handler.html is for navigator.mozSetMessageHandler registration. * But handler.html is not embedded in index.html in an iframe inside index.html * If the app is currently running, with this gecko fix, it won't finally receive any system message. If the app is currently running, without this patch, its UI page would be replaced by system app's |iframe.src = evt.detail.url;| which is very bad. The original design is for the case that expecting app using index.html for main UI and index.html#message for handling system message (by hashchange event), though there's no this use case now. We should remove the replacement in system app I think. But I'm not sure. App could do any implementation way they want...we need clear usage about how to deal with system message. Fabrice, thought?

(In reply to Alive Kuo [:alive] from comment #17) > I have no strong opinion if this change doesn't break current any system > message handler. > > BTW, I think we don't have a clear spec about how app should deal with > system message. > Think about this case: > (1) index.html is for UI rendering > (2) handler.html is for navigator.mozSetMessageHandler registration. > * But handler.html is not embedded in index.html in an iframe inside > index.html * > > If the app is currently running, with this gecko fix, it won't finally > receive any system message. Nope, it depends on the Manifest. If the Manifest says the handler is "handler.html", Gecko will realize it is not running so it send the `open-app` event. The proposed fix realizes when the handler is loaded, not the application. It is very specific. > If the app is currently running, without this patch, its UI page would be > replaced by system app's > |iframe.src = evt.detail.url;| which is very bad. Nope, it depends on the current state of the application. If it is in foreground, it wont be replaced. Indeed, the message is dropped (but it is ack as well, which is another error from my point of view). But if it is in the background, then it will be replaced by the handler.html. > The original design is for the case that expecting app using index.html for > main UI and index.html#message for handling system message (by hashchange > event), though there's no this use case now. Then we must encourage this is the only accepted behavior, i.e. specifying only the hashtag. > We should remove the replacement in system app I think. This is not necessary. The only necessary thing is to not reopen the handler. To not send the open-app when handler is already running.

(In reply to Salvador de la Puente González [:salva] from comment #18) > (In reply to Alive Kuo [:alive] from comment #17) > > I have no strong opinion if this change doesn't break current any system > > message handler. > > > > BTW, I think we don't have a clear spec about how app should deal with > > system message. > > > Think about this case: > > (1) index.html is for UI rendering > > (2) handler.html is for navigator.mozSetMessageHandler registration. > > * But handler.html is not embedded in index.html in an iframe inside > > index.html * > > > > If the app is currently running, with this gecko fix, it won't finally > > receive any system message. > > Nope, it depends on the Manifest. If the Manifest says the handler is > "handler.html", Gecko will realize it is not running so it send the > `open-app` event. The proposed fix realizes when the handler is loaded, not > the application. It is very specific. This is all different from Gene told me. Gene ^? > > Nope, it depends on the current state of the application. If it is in > foreground, it wont be replaced. Indeed, the message is dropped (but it is > ack as well, which is another error from my point of view). But if it is in > the background, then it will be replaced by the handler.html. Ya you're right, but this is still strange that replacement(not only hash-change) occurs when app is background.

I think we're mixing and confusing things up. To try to sum up, the way it works currently is: 1 The parent process keeps a list of the loaded pages (windows/frames) on an app 2 If and only if the target URL (as defined on the manifest) is loaded on a frame, the parent process sends a message to the correct child process frame to handle the window 3 And the parent process sends *always* a open-app message to the system app The system app, on the other hand, do: A. If the app isn't running, launches it B. If the app is running, on the foreground, does nothing C. If the app is running, on the background, and the top frame is the handler URL, does nothing. D. If the app is running, on the background, and the top frame isn't the handler, then loads the handler as the top frame. When both 2 condition is true and D happens, the behavior is incorrect and undefined. What I propose to change fixes that case (and just that case).

After looking more into the system message design, I think Antonio's investigation is right. Actually, comment #20 doesn't consider the case that the handler.html is embedded in the index.html, sharing the same window/frame. In this case, assuming the app is running, the parent won't send (i.e. IPCify) the system message to the window because the running window's page URL (index.html) is not matched with the registration in the manifest (handler.html). In this case, we still need the 'open-app' to switch the app to the handler.html to run the .mozSetMessageHandler(), so we don't need to modify any codes in the window_manager.js for now. Antonio's patch is sufficient. Thanks for all the clarifications and please correct me if I'm wrong. :)

Comment on attachment 770717 [details] [diff] [review] Sample solution, not tested, just to show Review of attachment 770717 [details] [diff] [review]: ----------------------------------------------------------------- I know it's just a WIP so it's not yet complete. Just supporting some initiative reviews and comments. I'm fine with the patch but maybe you can ask Fabrice's review for double checks. System message is our core mechanism. We need more eyes on it. Btw, you also need to fix the codes in .broadcastMessage(). ::: dom/messages/SystemMessageInternal.js @@ +49,5 @@ > > + > +const NOT_ALLOWED_TO_SEND = 0; > +const MESSAGE_SENT = 1; > +const APP_NOT_RUNNING = 2; Maybe, let's do: const MSG_SEND_SUCCESS = 0; const MSG_SEND_FAILURE_PERM_DENIED = 1; const MSG_SEND_FAILURE_APP_NOT_RUNNING = 2; @@ +153,5 @@ > " for " + aPageURI.spec + " @ " + aManifestURI.spec); > > // Don't need to open the pages and queue the system message > // which was not allowed to be sent. > + let messageSent = this._sendMessageCommon(aType, s/messageSent/sendResult(or just result) @@ +158,4 @@ > aMessage, > messageID, > aPageURI.spec, > + aManifestURI.spec); I know it's just WIP. Just a reminder to align this parameters. @@ +158,5 @@ > aMessage, > messageID, > aPageURI.spec, > + aManifestURI.spec); > + if (!messageSent) { if (result === MSG_SEND_FAILURE_PERM_DENIED) @@ +175,5 @@ > // Open app pages to handle their pending messages. > // Note that we only need to open each app page once. > let key = this._createKeyForPage(aPage); > if (!pagesToOpen.hasOwnProperty(key)) { > + if (messageSent === APP_NOT_RUNNING) { if (result === MSG_SEND_FAILURE_APP_NOT_RUNNING) { @@ +183,1 @@ > pagesToOpen[key] = true; Please also put the above line into the if-block. This is to avoid opening app redundantly. Note that we recently have a big refactoring task at Bug 874339. I'm afraid you need to make central/b2g18 specific patches here or let's ask for uplifting Bug 874339 first. @@ +541,5 @@ > if (!SystemMessagePermissionsChecker > .isSystemMessagePermittedToSend(aType, > aPageURI, > aManifestURI)) { > + return NOT_ALLOWED_TO_SEND; MSG_SEND_FAILURE_PERM_DENIED @@ +587,5 @@ > // expect that we will receive a "SystemMessageManager:HandleMessagesDone" > // message when the page finishes handling the system message with other > // pending messages. At that point, we'll release the lock we acquired. > this._acquireCpuWakeLock(pageKey); > + return APP_NOT_RUNNING; MSG_SEND_FAILURE_APP_NOT_RUNNING @@ +589,5 @@ > // pending messages. At that point, we'll release the lock we acquired. > this._acquireCpuWakeLock(pageKey); > + return APP_NOT_RUNNING; > + } else { > + return MESSAGE_SENT; MSG_SEND_SUCCESS

s/MSG_SEND_*/MSG_SENT_*/ might be better.

This patch applies over master, if/when it's reviewed I'll do the 1.1 version. I've tried it with some of the existing apps and with Salvador's test app: * If the app has a frame loaded with the handler, the handler is invoked and the app handler isn't loaded again. * If the app doesn't have a frame loaded with the handler and it's in background, then the handler is loaded at the top frame before sending the message.

Comment on attachment 771410 [details] [diff] [review] Does not send the open-app when the message has been delivered already, V1 Review of attachment 771410 [details] [diff] [review]: ----------------------------------------------------------------- Please remember to add Bug 888926 - XXX in front of the HG commit message when you're ready to land. Nice work! Thanks for all the verification and clarification. r=gene ::: dom/messages/SystemMessageInternal.js @@ +163,5 @@ > debug("Sending " + aType + " " + JSON.stringify(aMessage) + > " for " + aPageURI.spec + " @ " + aManifestURI.spec); > > // Don't need to open the pages and queue the system message > // which was not allowed to be sent. Please move the comments to below. @@ +170,5 @@ > + messageID, > + aPageURI.spec, > + aManifestURI.spec); > + > + if (result === MSG_SENT_FAILURE_PERM_DENIED) { Please move the above comments to the line before this condition and add a debug message: let result = this._sendMessageCommon(...); debug("The returned status of sending message: " + result); // Don't need to open the pages and queue the system message // which was not allowed to be sent. if (result === MSG_SENT_FAILURE_PERM_DENIED) { return; } @@ +180,5 @@ > // Queue this message in the corresponding pages. > this._queueMessage(page, aMessage, messageID); > > + if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) { > + // Don't open the page again if we already sent the message to it Nit: please add a period at the end of the comment sentence. In general, the comment should be in the format of: //{one-space}{upper-case-letter}{...}{period} @@ +205,5 @@ > // Find pages that registered an handler for this type. > this._pages.forEach(function(aPage) { > if (aPage.type == aType) { > // Don't need to open the pages and queue the system message > // which was not allowed to be sent. Please move the comments to below. @@ +211,5 @@ > + aMessage, > + messageID, > + aPage.uri, > + aPage.manifest); > + if (result === MSG_SENT_FAILURE_PERM_DENIED) { Ditto. Please move the above comments to the line before this condition and add a debug message: let result = this._sendMessageCommon(...); debug("The returned status of sending message: " + result); // Don't need to open the pages and queue the system message // which was not allowed to be sent. if (result === MSG_SENT_FAILURE_PERM_DENIED) { return; }

Thank you very much for solving this bug. This not only fixes the Cost Control problem but involves a lot of code to be removed, code that was there to try to minimize consequences of the incorrect behavior.

Thanks for the review, Gene. I have a new version with your changes, but waiting for Fabrice's review before uploading it.

Comment on attachment 771410 [details] [diff] [review] Does not send the open-app when the message has been delivered already, V1 Review of attachment 771410 [details] [diff] [review]: ----------------------------------------------------------------- Looks good to me. I'm not sure that gaia is doing the right thing in all situations, but we'll need to clear that up later.

Includes the comments from Gene's review. Keeping r+, r=gene.lian, r=fabrice.

This is the same patch, but for B2G18 (the other one doesn't apply cleanly since SystemMessages has been changed).

Comment on attachment 772346 [details] [diff] [review] Patch for b2g18 Review of attachment 772346 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/messages/SystemMessageInternal.js @@ +177,5 @@ > // Open app pages to handle their pending messages. > // Note that we only need to open each app page once. > let key = this._createKeyForPage(aPage); > if (!pagesToOpen.hasOwnProperty(key)) { > + if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) { That would be a bonus if we can merge these 2 conditions but it's not really a big matter. if (!pagesToOpen.hasOwnProperty(key) && result === MSG_SENT_FAILURE_APP_NOT_RUNNING) { ... } @@ +225,5 @@ > // Open app pages to handle their pending messages. > // Note that we only need to open each app page once. > let key = this._createKeyForPage(aPage); > if (!pagesToOpen.hasOwnProperty(key)) { > + if (result === MSG_SENT_FAILURE_APP_NOT_RUNNING) { Ditto.

Thanks for the review, Gene and Fabrice. Letting the B2G18 as is if you don't mind since that way is as similar as possible to the m-c version.

After testing this on Inari, it finally solved bug 882084 :)

What I'm worrying is happening, unfortunately. This bug might cause a regression at bug 893703.