Click events are blocked during waiting for response of modelprompt

RESOLVED FIXED in Firefox 34, Firefox OS v2.0

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ankit93040, Assigned: kk1fff)

Tracking

({regression})

unspecified
mozilla34
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:2.0+, firefox32 wontfix, firefox33 wontfix, firefox34 fixed, b2g-v1.4 unaffected, b2g-v2.0 verified, b2g-v2.1 verified)

Details

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

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

4 years ago
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.
(Reporter)

Updated

4 years ago
Whiteboard: [g+][LibGLA, Dev, B]
(Reporter)

Updated

4 years ago
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)
(Reporter)

Comment 2

4 years ago
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?
status-b2g-v1.4: --- → unaffected
status-b2g-v2.0: --- → affected
status-b2g-v2.1: --- → affected
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.
Created attachment 8467109 [details] [diff] [review]
debug-alert.patch

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

Updated

4 years ago
Keywords: regressionwindow-wanted
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.
Blocks: 936729
Keywords: regressionwindow-wanted
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
(Assignee)

Updated

4 years ago
Summary: Touch events are blocked during waiting for response of modelprompt → Click events are blocked during waiting for response of modelprompt
Created attachment 8469187 [details] [diff] [review]
Proposed patch

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.

Updated

4 years ago
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)
Created attachment 8469906 [details] [diff] [review]
debug-alert-2

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)
Created attachment 8469951 [details] [diff] [review]
Proposed Patch v.2

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)

Updated

4 years ago
QA Whiteboard: [2.0-signoff-need]

Updated

4 years ago
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.
Created attachment 8477547 [details] [diff] [review]
Proposed Patch v.3
Attachment #8469951 - Attachment is obsolete: true
Created attachment 8477554 [details] [diff] [review]
Proposed Patch v.4
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+
Created attachment 8477972 [details] [diff] [review]
Proposed Patch v.5 (r=bent)

update according to review comment.
Attachment #8477554 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/98623c0c5ba2
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/cb98ddca2e97
status-b2g-v2.0: affected → fixed
status-b2g-v2.1: affected → fixed
status-firefox32: --- → wontfix
status-firefox33: --- → wontfix
status-firefox34: --- → fixed

Comment 29

4 years ago
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
status-b2g-v2.0: fixed → verified
status-b2g-v2.1: fixed → verified

Comment 30

4 years ago
Created attachment 8532980 [details]
Verify_video.3gp
You need to log in before you can comment on or make changes to this bug.