[GitHub issue by mozillamarcia on 2012-09-27T22:25:18Z, https://github.com/mozilla-b2g/gaia/issues/5337] Otoro phone, build 2012-09-26 us Taken from default.xml in b2g-distro: "platform_build" revision= 7952619 "gaia" revision= 34bcd07 "releases-mozilla-central" revision= 35d92aa "gonk-misc" revision= 1fa9534 Repro : 1. Long press on the home screen until you get the "Pick" screen. 2. Select "Camera" from the list Actual : App crashes and phone reboots Expected : App would not crash. Notes: Similiar issue with Gallery, but does not happen every time. Selecting Wallpaper works fine.

[GitHub comment by nhirata on 2012-09-27T23:13:31Z] oops. did not mean to close this issue.

[GitHub comment by autonome on 2012-09-28T07:10:17Z] /cc @daleharvey @davidflanagan

[GitHub comment by jds2501 on 2012-09-29T02:01:02Z] A crash is a gecko bug. Can you move this to bugzilla under boottogecko --> general?

Moving as requested in Comment 3.

https://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#472 crashes because aFrameLoader is 0 Is it normal that TabParent->mFrameElement is 0 ? I don't have enough information to fix this bug yet. /me investigating

No, that is not expected.

I don't know if this is right but it seems that we have multiple pages registered for the camera app. This 'looks' right because we have 2 activities (record and pick). But when the pick 'signal' is emitted, these 2 pages are activated and the first one removes its frame. When the second page handles the same signal, the frame is gone, and so the mFrameElement is null. Crash. If this is not the right way to fix the problem, give me a feedback and I can continue.

Comment on attachment 669540 [details] [diff] [review] patch 1 Review of attachment 669540 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/TabParent.cpp @@ +1021,5 @@ > { > MOZ_ASSERT(ManagedPRenderFrameParent().IsEmpty()); > > nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader(); > + MOZ_ASSERT(frameLoader); Not sure if this is better than https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=798980&attachment=669616 ::: dom/messages/SystemMessageInternal.js @@ +117,5 @@ > throw Cr.NS_ERROR_INVALID_ARG; > } > > + let found = false; > + for (let page in this._pages) { Nit: you could use this._pages.some(...) here.

(In reply to Fabrice Desré [:fabrice] from comment #8) > Comment on attachment 669540 [details] [diff] [review] > patch 1 > > Review of attachment 669540 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/ipc/TabParent.cpp > @@ +1021,5 @@ > > { > > MOZ_ASSERT(ManagedPRenderFrameParent().IsEmpty()); > > > > nsRefPtr<nsFrameLoader> frameLoader = GetFrameLoader(); > > + MOZ_ASSERT(frameLoader); > > Not sure if this is better than > https://bugzilla.mozilla.org/page.cgi?id=splinter. > html&bug=798980&attachment=669616 > They're solving different problems. We should always have a frame loader here under normal operation, but if we don't, we shouldn't crash the main b2g process. I think jet's patch is sufficient for that, and we can remove the assertion here. (The subprocess will crash and the main process with NS_ERROR().)

I used to have the exactly same solution at: https://bugzilla.mozilla.org/attachment.cgi?id=658454&action=diff for Bug 788466, but it wasn't accepted due to some considerations at https://bugzilla.mozilla.org/show_bug.cgi?id=788466#c5. If we're fine with it now, I'm glad to close that bug. ;)

backed out because https://bugzilla.mozilla.org/show_bug.cgi?id=788466#c5 is actually right... https://hg.mozilla.org/integration/mozilla-inbound/rev/a7acfcb014f9

(In reply to Fabrice Desré [:fabrice] from comment #14) > backed out because https://bugzilla.mozilla.org/show_bug.cgi?id=788466#c5 is > actually right... Hi Fabrice, After more investigation, I think it's reasonable to avoid duplicate pages (with the same {type, manifest, url} to be registered in the System Message, because from the point of view of "System Message", it only cares about "where" to send the messages. Supposing we have the following example: "activities": { "pick": { "filters": { "type": "webcontacts/contact" }, "disposition": "window", "href": "/contacts/index.html", "returnValue": true }, "new": { "filters": { "type": "webcontacts/contact" }, "disposition": "window", "href": "/contacts/index.html", "returnValue": true } } Both these 2 activities have the same page: page.type = "activity" page.url = "/contact/index.html" page.manifest = "app://comm.gaiamobile.org/manifest.webapp" If we only register this page in the system message once, when the Gaia end starts a "pick" activity, the system message will attempt to open the page "once" and also send/handle the message "once" (another "new" activity will also do exactly the same thing). I'm still a bit confused why we need to have multiple page entries registered for each activity in the System Message back-end? Could you please point out a practical example that would have bad effects if we don't register distinct page for each activity? I could be wrong. Please feel free to correct me. :)

Btw, I think the issue you mentioned on the IRC that the homescreen app didn't show up is due to another potential wrong logic at [1]. We shouldn't return there! If we do, only the second attempt of opening homescreen app will succeed to display. Note that opening apps twice is exactly the issue we're talking about here. It's a mistake made based on another mistake ;) What do you think? [1] https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1091

Gene, I still think that it would be better to have one pending queue per activity, even if they register the same system message, but I have no clean way to do that. Ideally, instead of registering an "activity" message, we should register a "activity-view", "activity-pick" message. But then apps would have to set a handler for these messages, and not for an "activity" message.

Hi Fabrice, I upload another patch, which tries to de-duplicate the pages before opening them. I think doing this is reasonable, because there is no reason to open the SAME app page more than twice to handle its pending messages (one time is enough even if we have multiple messages to be sent). I think I could understand your concerns: from the point of view of system message mechanism, it sounds reasonable to keep pending messages for each activity. However, from the point of view of app-opening behavior, we don't need to open the page twice, which sounds a bit redundant. Therefore, let's summarize the pages to be opened in the long run before opening them. Could you please take a review on this patch? Please correct me if I'm wrong. Thanks! :)

We also need to have corresponding changes in window_manager.js: https://github.com/mozilla-b2g/gaia/pull/5789

Hi Fabrice, ping a bit on this bug. Are you OK to my solution at comment 19 and comment 20? :) This issue is blocking our camera app unfortunately. ----- I'd like to try again to convince you because I still think de-duplicating the pages when registering them is the best solution to me. Because the system message mechanism only needs to care about how to route the messages (where to send them) and shouldn't be aware of who (like activities) registers the pages, so we don't need to set up different pending message queues for each activity. If you still prefer keeping multiple pending queues (all of them have the same page), there might be some potential errors that I could see so far: 1. SystemMessageInternal.broadcastMessage() is wrong because it would fire message to the same page more than once [1]. 2. "SystemMessageManager:GetPendingMessages" is wrong because it only clears one pending message queue if we use _pages.some() [2]. 3. It would try to open the same page more than twice, which is just the root cause of this camera issue [3]. [1] https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#117 [2] https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#184 [3] https://mxr.mozilla.org/mozilla-central/source/dom/messages/SystemMessageInternal.js#92

(In reply to Gene Lian [:gene] from comment #20) > We also need to have corresponding changes in window_manager.js: > > https://github.com/mozilla-b2g/gaia/pull/5789 Yes, that's an logic error. Please rewrite the commit comment and r? me so I can merge this.

Gene, here's a (not finished) patch that explores another option: sendMessage() and broadcastMessage() have a new |customType| optional parameter. We still use the |type| to do the matching, but if a custom type is specified, we fire this kind of event. That let activities be registered with a type of "activity-view", "activity-pick" while still being dispatched as activities. With this patch we launch the homescreen but we fail in hasPendingMessages(). Can you take a look? I think it's overall a better solution but I'm not 100% sure we can make that work while still only registering for "activity" on the web content side. If that doesn't work, we'll go with your latest patch (which needs to be rebased but looks ok otherwise).

(In reply to Fabrice Desré [:fabrice] from comment #24) > With this patch we launch the homescreen but we fail in > hasPendingMessages(). Can you take a look? I think it's overall a better > solution but I'm not 100% sure we can make that work while still only > registering for "activity" on the web content side. If that doesn't work, > we'll go with your latest patch (which needs to be rebased but looks ok > otherwise). Hmm... I think that's possible. Although the app-side can only do the following thing: window.navigator.mozSetMessageHandler('activity', function(){}) we can still fetch the pending messages and handle them when the "prefix part" is matched (that is, the "activity" part here). However, we can only match them by comparing the prefix because System Message cannot do hard-code things specifically for activities. If you agree on the prefix comparing method, I can support implement that and come back with the patch. :) What do you think?

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #23) > (In reply to Gene Lian [:gene] from comment #20) > > We also need to have corresponding changes in window_manager.js: > > > > https://github.com/mozilla-b2g/gaia/pull/5789 > > Yes, that's an logic error. Please rewrite the commit comment and r? me so I > can merge this. Done. Please see https://github.com/mozilla-b2g/gaia/pull/5876 We need this change on the Gaia end, which can go first without considering the Gecko changes since it's an obvious logic error.

(In reply to Gene Lian [:gene] from comment #26) > (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #23) > > (In reply to Gene Lian [:gene] from comment #20) > > > We also need to have corresponding changes in window_manager.js: > > > > > > https://github.com/mozilla-b2g/gaia/pull/5789 > > > > Yes, that's an logic error. Please rewrite the commit comment and r? me so I > > can merge this. > > Done. Please see https://github.com/mozilla-b2g/gaia/pull/5876 > > We need this change on the Gaia end, which can go first without considering > the Gecko changes since it's an obvious logic error. Rewrite the commit message with a better description. Thanks Tim for the suggestion! :) https://github.com/mozilla-b2g/gaia/pull/5879

(In reply to Gene Lian [:gene] from comment #27) > Rewrite the commit message with a better description. Thanks Tim for the > suggestion! :) > > https://github.com/mozilla-b2g/gaia/pull/5879 Gaia part merged.

Hi Fabrice, Just like what we discussed on the IRC, let's work it around with this patch first. In the long run, we'll ask apps to use "activity-XXX" to register message handler. Need your review on this patch. Thanks! :)

Ryan, I'm afraid this needs to be checked in after Bug 801573. What should we do now?

(In reply to Gene Lian [:gene] from comment #34) > Ryan, > > I'm afraid this needs to be checked in after Bug 801573. What should we do > now? Well, I think I'll post another aurora-specific patch for Bug 801573 to avoid conflict, since this one has been landed first. No worries!

Bug 801573 just got bb+ status, so I'll just back this cset out and re-land it on top of that.

(In the future, though, it would help if dependencies or landing orders were noted on the whiteboard to avoid confusion)

Backout: https://hg.mozilla.org/releases/mozilla-aurora/rev/edadf84382ce Re-landed: https://hg.mozilla.org/releases/mozilla-aurora/rev/b0b68a17a503