Closed Bug 1047303 Opened 10 years ago Closed 10 years ago

Click events are blocked during waiting for response of modelprompt

Categories

(Core :: DOM: Content Processes, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla34
blocking-b2g 2.0+
Tracking Status
firefox32 --- wontfix
firefox33 --- wontfix
firefox34 --- fixed
b2g-v1.4 --- unaffected
b2g-v2.0 --- verified
b2g-v2.1 --- verified

People

(Reporter: ankit93040, Assigned: kk1fff)

References

Details

(Keywords: regression, Whiteboard: [g+][LibGLA, Dev, B] )

Attachments

(4 files, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36 Steps to reproduce: 1. Go to music 2. select a music file more than 600Kb (more than mms size) 3. share the music via message 4. Gets an alert message. (Don't press ok). 5. Press home button. 6. Go to message Actual results: Message operation are not working Expected results: Message operation should work. Go back to music and press ok for the alert in step 4. Message works. Alert should diminish itself when visibility is changed.
Whiteboard: [g+][LibGLA, Dev, B]
Flags: needinfo?(felash)
Ankit, can you please share your Firefox OS version? (ideally, version + build id).
Component: General → Gaia::System::Window Mgmt
Flags: needinfo?(felash) → needinfo?(ankit93040)
I used Mozilla master. Gecko - central. Even today it is reproducioble.
Flags: needinfo?(ankit93040)
Ok, I see the issue. Here is what happens from the logs I added: * we use window.alert to show this warning, in the activity window * when a page uses window.alert, it doesn't get (some ?) events anymore * it seems this happens also when we have different windows here (as we have 1 window for the activity and 1 window for when the user launches Messages again). Vivien, Alive, is it supposed to happen like this? If yes, then we'll need to stop using window.alert and window.confirm... [Blocking Requested - why for this release]: this happens in v2.0 too, but not in v1.4 (because we don't do the same thing in that case).
blocking-b2g: --- → 2.0?
Component: Gaia::System::Window Mgmt → Gaia::SMS
Flags: needinfo?(alive)
Flags: needinfo?(21)
Keywords: regression
I discussed about this with Vivien, it seems to be an issue quite deep in the event loop in Gecko. I reproduced the same deep issue in v1.4, so it looks like this comes from some time ago... maybe it was always there, and only know when we use it we see it's broken. I'm investigating more.
STR: 1. apply this patch 2. push the "communications" app 3. monitor logcat 4. launch both dialer and contacts 5. connect on "communications" using Firefox' devtools' WebIDE 6. see in the inspector which app is connected (either contacts or dialer) 7. run "window.alert('foobar')" in the console 8. watch logcat => you'll see the "setInterval" logs are stopping for the _other_ app => if you try to manipulate the other app, you'll see the mouse events are not working, but the touch events are
Olli and Vivien helped me a lot here, and we think there is an issue in how the maps work in [1]. It looks like we don't get the right window. Patrick, will you be able to help here? [1] https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPromptService.jsm#609
Flags: needinfo?(kk1fff)
Flags: needinfo?(alive)
Flags: needinfo?(21)
Summary: [SMS][Music] Sms gets locked → [SMS] Sms gets locked when an alert is sent in one window
Status: UNCONFIRMED → NEW
Ever confirmed: true
Jason, I think the regression will come from bug 936729.
(In reply to Julien Wajsberg [:julienw] from comment #7) > Jason, I think the regression will come from bug 936729. Ok, sounds good. I'll remove the window request then if we have an idea of the cause.
blocking-b2g: 2.0? → 2.0+
(In reply to Julien Wajsberg [:julienw] from comment #6) > the maps work in [1]. It looks like we don't get the right window. > [1] > https://mxr.mozilla.org/mozilla-central/source/dom/browser-element/ > BrowserElementPromptService.jsm#609 A BEC is inserted into the map when it is created. According to the STR in comment 0, there's only one BEC in the map when SMS app try to show an alert. Thus I don't think it got wrong window or sent showmodelprompt to wrong BEP. I suspect that events in the new window (opened by clicking message app) is blocked because there's another BEC in the process is waiting for the response to showmodelprompt[1]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#421
Flags: needinfo?(kk1fff)
Patrick, in the STR in comment 5, both windows are open when I call "window.alert". So maybe this is the same bug, but 2 different causes? Can you fix this or should we change something elsewhere?
Flags: needinfo?(kk1fff)
I think that what comment 5 says includes: 1) setTimeout events are blocked when an alert box is shown in the tab. I am not pretty sure what behavior is expected here, but the behavior is the same as what desktop browser do. and 2) the click events are not dispatched (but queued) to the tab in which the alert box is not shown, I think this is the cause of bug described in comment 0. It looks like the bug is caused by the nested loop we use to block the alert() call. Although the nested loop runs nsThread tasks, it doesn't process tasks that are dispatched to MessageLoop, where click event is dispatched to [1]. [1] http://dxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.cpp#1824
Assignee: nobody → kk1fff
Component: Gaia::SMS → General
Flags: needinfo?(kk1fff)
Summary: [SMS] Sms gets locked when an alert is sent in one window → Touch events are blocked during waiting for response of modelprompt
Summary: Touch events are blocked during waiting for response of modelprompt → Click events are blocked during waiting for response of modelprompt
Attached patch Proposed patch (obsolete) — Splinter Review
Use nsITimer to delay click events.
Attachment #8469187 - Flags: review?(khuey)
(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #11) > I think that what comment 5 says includes: 1) setTimeout events are blocked > when an alert box is shown in the tab. I am not pretty sure what behavior is > expected here, but the behavior is the same as what desktop browser do. and Actually, what happens is that setInterval events are blocked in the tab that _does_ _not_ have the alert box, and are _not_ blocked on the other tab. I couldn't run a alert on both tabs to check this, but I expect that the same tab is blocking the events, when alert is run in any of it. So depending on which tab runs the alert, sometimes the expected behaviour works and sometimes it doesn't.
Component: General → IPC
Product: Firefox OS → Core
Component: IPC → DOM: Content Processes
(In reply to Julien Wajsberg [:julienw] from comment #13) > Actually, what happens is that setInterval events are blocked in the tab > that _does_ _not_ have the alert box, and are _not_ blocked on the other tab. Well.. What I see with the patch is that setInterval is blocked if the tab has an alert. My STR is: 1. open dailer 2. open contacts 3. debug communication in app manager. 4. type 'location.herf' in console, get 'app://communications.gaiamobile.org/dialer/index.html#keyboard-view', which means our commands go to dialer's tab (looks like they always go to the first opened tab, although it has been pushed to background). 5. type 'alert("xx")', and log of dialer stops. 6. on device, back to dailer app, I can see an alert box shown on it. 7. tap 'ok' to close alert box, log of dialer continues.
(In reply to Patrick Wang (Chih-Kai Wang) (:kk1fff) from comment #14) > (In reply to Julien Wajsberg [:julienw] from comment #13) > > Actually, what happens is that setInterval events are blocked in the tab > > that _does_ _not_ have the alert box, and are _not_ blocked on the other tab. > > Well.. What I see with the patch is that setInterval is blocked if the tab > has an alert. My STR is: > 1. open dailer > 2. open contacts > 3. debug communication in app manager. > 4. type 'location.herf' in console, get > 'app://communications.gaiamobile.org/dialer/index.html#keyboard-view', which > means our commands go to dialer's tab (looks like they always go to the > first opened tab, although it has been pushed to background). > 5. type 'alert("xx")', and log of dialer stops. > 6. on device, back to dailer app, I can see an alert box shown on it. > 7. tap 'ok' to close alert box, log of dialer continues. as I said, I think it depends which application the app manager gives you... To check this properly we'd need to hack a button in the apps that would start an alert, so that we don't need App Manager. NI me to do this.
Flags: needinfo?(felash)
Attached patch debug-alert-2Splinter Review
This debug patch starts a "alert" box whenever a click event happens. I actually don't see the behavior I saw in comment 5, which puzzles me.
Flags: needinfo?(felash)
Attached patch Proposed Patch v.2 (obsolete) — Splinter Review
Dispatch context menu event with nsITimer, too.
Attachment #8469187 - Attachment is obsolete: true
Attachment #8469187 - Flags: review?(khuey)
Attachment #8469951 - Flags: review?(khuey)
Comment on attachment 8469951 [details] [diff] [review] Proposed Patch v.2 I'll take this.
Attachment #8469951 - Flags: review?(khuey) → review?(bent.mozilla)
QA Whiteboard: [2.0-signoff-need]
QA Whiteboard: [2.0-signoff-need] → [2.0-signoff-need+]
Comment on attachment 8469951 [details] [diff] [review] Proposed Patch v.2 Review of attachment 8469951 [details] [diff] [review]: ----------------------------------------------------------------- I think the lifetime issues here require another patch unfortunately. ::: dom/ipc/TabChild.cpp @@ +114,5 @@ > static TabChildMap* sTabChildren; > > +namespace { > + > +class DelayedCallback: public nsITimerCallback { Nit: Please make sure you format like this: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes @@ +121,5 @@ > + NS_DECL_NSITIMERCALLBACK > + DelayedCallback() { } > + > + already_AddRefed<nsITimer> > + GetTimer(uint32_t delay) { Nit: I'd call this "CreateTimer" since that's more what it's doing. Also all args get a prefix, so this should be "aDelay" @@ +127,5 @@ > + timer->InitWithCallback(this, delay, nsITimer::TYPE_ONE_SHOT); > + return timer.forget(); > + } > + > +protected: This needs a protected virtual destructor. @@ +128,5 @@ > + return timer.forget(); > + } > + > +protected: > + virtual void DoCallback() = 0; It doesn't look like this buys you anything, why not just have the subclasses overrive Notify themselves? @@ +141,5 @@ > +} > + > +} // anonymous namespace > + > +class TabChild::DelayedFireSingleTapEvent: public DelayedCallback { Each of these subclasses needs to me marked as MOZ_FINAL and have a private destructor. @@ +158,5 @@ > + mTabChild->FireSingleTapEvent(mPoint); > + mTimer = nullptr; > + } > + > + TabChild *mTabChild; What guarantees that this TabChild stays alive long enough for the timer to run? There are two possible problems here: 1. The TabChild object is deleted and mTabChild is a dangling pointer. CLearly that's just going to crash. 2. The TabChild object has been told to destroy itself by the parent (see TabChild::RecvDestroy). I don't know what should happen in this case, do you still want to call FireSingleTapEvent()/FireContextMenuEvent()? @@ +163,5 @@ > + LayoutDevicePoint mPoint; > + nsCOMPtr<nsITimer> mTimer; > +}; > + > +class TabChild::DelayedFireContextMenuEvent: public DelayedCallback { This one isn't holding the timer alive so I doubt it ever runs... @@ +173,5 @@ > + void DoCallback() { > + mTabChild->FireContextMenuEvent(); > + } > + > + TabChild *mTabChild; Similar problem with this one. ::: dom/ipc/TabChild.h @@ +579,5 @@ > int32_t mActivePointerId; > // A timer task that fires if the tap-hold timeout is exceeded by > // the touch we're tracking. That is, if touchend or a touchmove > // that exceeds the gesture threshold doesn't happen. > + nsCOMPtr<nsITimer> mTapHoldTimer; Since this is now a nsCOMPtr please remove its initializer entry in TabChild's constructor.
Attachment #8469951 - Flags: review?(bent.mozilla) → review-
Thanks for the review and comments, Ben. They are helpful. I'll update patch these days.
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #19) > @@ +158,5 @@ > > + mTabChild->FireSingleTapEvent(mPoint); > > + mTimer = nullptr; > > + } > > + > > + TabChild *mTabChild; > > What guarantees that this TabChild stays alive long enough for the timer to > run? There are two possible problems here: > > 1. The TabChild object is deleted and mTabChild is a dangling pointer. > CLearly that's just going to crash. > 2. The TabChild object has been told to destroy itself by the parent (see > TabChild::RecvDestroy). I don't know what should happen in this case, do you > still want to call FireSingleTapEvent()/FireContextMenuEvent()? These two events are from IPC messages. If TabChild's IPC protocol is disconnected during the delay, I think the delayed events should be cancelled. > @@ +163,5 @@ > > + LayoutDevicePoint mPoint; > > + nsCOMPtr<nsITimer> mTimer; > > +}; > > + > > +class TabChild::DelayedFireContextMenuEvent: public DelayedCallback { > > This one isn't holding the timer alive so I doubt it ever runs... This timer is held by TabChild.mTapHoldTimer, because TabChild wants to control the timer. > > @@ +173,5 @@ > > + void DoCallback() { > > + mTabChild->FireContextMenuEvent(); > > + } > > + > > + TabChild *mTabChild; > > Similar problem with this one. This callback object is held by a timer object which is held by TabChild. If TabChild dies, this object will be deleted as well.
Attached patch Proposed Patch v.3 (obsolete) — Splinter Review
Attachment #8469951 - Attachment is obsolete: true
Attached patch Proposed Patch v.4 (obsolete) — Splinter Review
Attachment #8477547 - Attachment is obsolete: true
Attachment #8477554 - Flags: review?(bent.mozilla)
Comment on attachment 8477554 [details] [diff] [review] Proposed Patch v.4 Review of attachment 8477554 [details] [diff] [review]: ----------------------------------------------------------------- r=me with these fixed: ::: dom/ipc/TabChild.cpp @@ +112,5 @@ > > typedef nsDataHashtable<nsUint64HashKey, TabChild*> TabChildMap; > static TabChildMap* sTabChildren; > > +class TabChild::DelayedFireSingleTapEvent : public nsITimerCallback Nit: Both of these classes should be MOZ_FINAL and both should have a private destructor. @@ +117,5 @@ > +{ > +public: > + NS_DECL_ISUPPORTS > + > + DelayedFireSingleTapEvent(TabChild *aTabChild, Nit: * goes on the left, here and below. @@ +120,5 @@ > + > + DelayedFireSingleTapEvent(TabChild *aTabChild, > + LayoutDevicePoint& aPoint, > + nsITimer *aTimer) > + : mTabChild(do_GetWeakReference((nsITabChild*)aTabChild)) Please use static_cast<> @@ +163,5 @@ > + return NS_OK; > + } > + > +private: > + TabChild *mTabChild; Please add a comment to document why it's safe to hold a raw pointer here. @@ +1870,5 @@ > LayoutDevicePoint currentPoint = APZCCallbackHelper::ApplyCallbackTransform(aPoint, aGuid) * mWidget->GetDefaultScale();; > + nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID); > + nsRefPtr<DelayedFireSingleTapEvent> callback = > + new DelayedFireSingleTapEvent(this, currentPoint, timer); > + timer->InitWithCallback(callback, If this call fails for some reason you'll leak both |timer| and |callback|. Please check for failure and ensure that nothing leaks if it does fail. @@ +2535,5 @@ > for (uint32_t i = 0; i < idbActors.Length(); ++i) { > static_cast<IndexedDBChild*>(idbActors[i])->Disconnect(); > } > > + mDestroyed = true; I think this should probably go at the top of the method, along with an assertion that mDestroyed is false before setting it to true.
Attachment #8477554 - Flags: review?(bent.mozilla) → review+
update according to review comment.
Attachment #8477554 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
This issue has been verified successfully on Flame 2.0,2.1 See attachment: Verify_video.3gp Reproducing rate: 0/5 Flame 2.0 build: Gaia-Rev 856863962362030174bae4e03d59c3ebbc182473 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/e40fe21e37f1 Build-ID 20141207000206 Version 32.0 Flame2.1 build: Gaia-Rev 38e17b0219cbc50a4ad6f51101898f89e513a552 Gecko-Rev https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/8b92c4b8f59a Build-ID 20141205001201 Version 34.0
Attached video Verify_video.3gp
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: