Double Tab III: Son of Double Tab (Opening a new tab using the overlay while a website is loading creates two about:start tabs)

RESOLVED WONTFIX

Status

defect
P1
normal
RESOLVED WONTFIX
6 years ago
3 months ago

People

(Reporter: kjozwiak, Unassigned)

Tracking

unspecified
x86_64
Windows 8.1
Dependency tree / graph

Firefox Tracking Flags

(firefox27 unaffected, firefox28- affected, firefox29- affected, firefox30- affected)

Details

(Whiteboard: p=5 s=it-30c-29a-28b.3 r=ff29)

Attachments

(2 attachments, 4 obsolete attachments)

Reporter

Description

6 years ago
When you tap on the "+" overlay while a website is being loaded, two different about:start tabs will be created rather than just a single tab. I reproduced this several times and attached a short video.

- Attached a video to illustrate the issue. Notice that when I use the overlay when polygon/facebook is loading, two about:start tabs are being created at the same time

Steps to reproduce the issue:

1) Open a fresh instance of Firefox Metro (I cleared everything using the "Options" flyout)
2) Type in "polygon.com" and press "Enter". Once the website starts loading, tap on the overlay "+" button and you'll notice two different about:start tabs will be created
3) Close all the tabs and go back to a single about:start screen
4) Type in "facebook.com" press "Enter". Once the website starts loading, tap on the "+" overlay button and you'll notice two different about:start tabs will be created

Current Behaviour:

- When taping on the "+" overlay while a website is being loaded, two different about:start tabs are being created rather than one

Expected Behaviour:

- One a single about:start tab should be created, not two different tabs.

Found the issue using the following build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-12-23-03-02-03-mozilla-central/
Maybe related to bug 942189 and the fact that we don't have an up-to-date APZC tree while the page is loading.
Depends on: 942189
Reporter

Comment 2

6 years ago
I was playing around with Facebook for about 10 minutes before opening a new tab using the overlay and two about:start tabs were created. Not sure if this is directly linked to loading but it does happen reliably during that loading phase.
Whiteboard: [triage] [defect] p=0 → [beta28] [defect] p=0
Assignee: nobody → rsilveira
Blocks: metrov1it22
No longer blocks: metrov1backlog
Status: NEW → ASSIGNED
Priority: -- → P2
QA Contact: jbecerra
Whiteboard: [beta28] [defect] p=0 → [beta28] [defect] p=1
marking wfm now that the parent bug is fixed.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → WORKSFORME
For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.
Reporter

Comment 5

5 years ago
This is definitely still happening but is really hard to reproduce. Sometimes when creating new tabs, two tabs will open rather than just the one. I will keep my eye on this when going through the next iteration and see if I can get some reliable STR.
Reporter

Comment 6

5 years ago
Moving this back into the backlog as I can reproduce the issue very consistently with both the X1 Carbon and the surface pro 2. I've attached a video to illustrate the issue:
- http://youtu.be/NgioUM6CUOA

Reproduced using the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-11-03-02-01-mozilla-central/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-02-11-00-40-37-mozilla-aurora/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/
Blocks: metrov1backlog
No longer blocks: metrov1it22
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Whiteboard: [beta28] [defect] p=1 → [triage] [defect] p=0
Assignee: rsilveira → nobody
QA Contact: jbecerra
Whiteboard: [triage] [defect] p=0 → [release28] p=0
Whiteboard: [release28] p=0 → [release28] p=0 r=ff28
This happened before in bug 936005 and bug 943451 because of issues with preventDefault for chrome touch events.  Bug 959792 could also be relevant.
Blocks: 936005, 943451
Summary: Opening a new tab using the overlay while a website is loading creates two about:start tabs → Double Tab III: Son of Double Tab (Opening a new tab using the overlay while a website is loading creates two about:start tabs)
Assignee: nobody → rsilveira
Posted patch 953012.patch (obsolete) — Splinter Review
Quoting mbrubeck on #windev[1]:
"maybe in perf-constrained situations, OnPointerReleased gets called before DeliverNextQueuedTouchEvent has had a chance to process the touchstart event."

"if we take too long to dequeue the pointerstart, then this code will not yet see that gesture recognition has been canceled [2]"

This fix moves the call mGestureRecognizer->ProcessUpEvent() inside DeliverNextQueuedTouchEvent() while processing a touch start or touch end event.

[1] - http://logbot.glob.com.au/?c=mozilla%23windev&s=25+Feb+2014&e=25+Feb+2014#c12984
[2] - http://dxr.mozilla.org/mozilla-ce...indows/winrt/MetroInput.cpp#712
Attachment #8382573 - Flags: feedback?(mbrubeck)
Status: REOPENED → ASSIGNED
Comment on attachment 8382573 [details] [diff] [review]
953012.patch

Review of attachment 8382573 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/windows/winrt/MetroInput.cpp
@@ +708,5 @@
>      new WidgetTouchEvent(true, NS_TOUCH_END, mWidget.Get());
>    touchEvent->touches.AppendElement(CreateDOMTouch(currentPoint.Get()));
>    DispatchAsyncTouchEvent(touchEvent);
>  
> +  mTouchEndPoint = currentPoint;

I think this could go wrong if OnPointerReleased is called again (for a second pointer) before the the first touch event has been dispatched.  To be guaranteed correct, we really need to add currentPoint to a queue (just like touchEvent).

Also, we should probably do the same for the other mGestureRecognizer->Process* calls, to avoid changing the order of events seen by the gesture recognizer.  Perhaps we could add a second argument to DispatchAsyncTouchEvent, and it could put both the WidgetTouchEvent and the IPointerPoint into a queue (or two parallel queues).  Or maybe there's a simpler way to do this that I haven't thought of...  (In JavaScript I would have DispatchAsyncTouchEvent return a promise, and just add a "yield" here.)
Attachment #8382573 - Flags: feedback?(mbrubeck) → feedback-
Priority: P2 → P1
QA Contact: kamiljoz
Whiteboard: [release28] p=0 r=ff28 → [release28] p=0 s=it-30c-29a-28b.3 r=ff28
Whiteboard: [release28] p=0 s=it-30c-29a-28b.3 r=ff28 → [release28] p=5 s=it-30c-29a-28b.3 r=ff28
Posted patch WIP (obsolete) — Splinter Review
Adding the gesture recognizer arguments to the input event queue and processing them when the touch event is delivered.

I'll do some cleanup and handle memory allocation properly in InputEventQueueEntry.
Attachment #8382573 - Attachment is obsolete: true
Posted patch WIP (obsolete) — Splinter Review
Forgot to qref.
Attachment #8386229 - Attachment is obsolete: true
Posted patch Patch v1 (obsolete) — Splinter Review
I tried simplifying the usage of TouchEventQueueEntry by storing IPointerEventArgs instead of having both IPointerPoint and PointerPointVector but the IPointerEventArgs passed to OnPointer* gets de-alocated after the method returns.
Attachment #8386237 - Attachment is obsolete: true
Attachment #8386510 - Flags: review?(mbrubeck)
Yes, we are going to build with our final beta today - let's get a nomination here for uplift with a breakdown of risk/reward.  Is this a metro-only fix?  That will make it more likely to be ok for uplift.
I'm not comfortable uplifting this to beta without some baking time in m-c. I don't think it meets the bar for beta at this point either.
Hey Rodrigo, can I remove Bug 953012 as a release 28 blocker?
(In reply to Rodrigo Silveira [:rsilveira] from comment #14)
> I'm not comfortable uplifting this to beta without some baking time in m-c.
> I don't think it meets the bar for beta at this point either.

Thank you for that assessment, we will not track this for FF28 in that case and it can get bake time on nightly (soon to be Aurora) with consideration for FF29 if safe enough.
Removing blocking tag and tagging for a FF29 release based on Comment #14 and #16.
Blocks: metrobacklog
No longer blocks: metrov1backlog
Whiteboard: [release28] p=5 s=it-30c-29a-28b.3 r=ff28 → p=5 s=it-30c-29a-28b.3 r=ff29
Comment on attachment 8386510 [details] [diff] [review]
Patch v1

Review of attachment 8386510 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, but I want Jim to do the final review on this.
Attachment #8386510 - Flags: review?(mbrubeck)
Attachment #8386510 - Flags: review?(jmathies)
Attachment #8386510 - Flags: feedback+

Updated

5 years ago
Component: App Bar → Widget: WinRT
Product: Firefox for Metro → Core
Comment on attachment 8386510 [details] [diff] [review]
Patch v1

Review of attachment 8386510 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks great, although there's one issue to fix.

::: widget/windows/winrt/MetroInput.cpp
@@ +656,5 @@
>      new WidgetTouchEvent(true, NS_TOUCH_MOVE, mWidget.Get());
>    InitTouchEventTouchList(touchEvent);
> +
> +  Foundation::Collections::IVector<UI::Input::PointerPoint*>* pointerPoints;
> +  aArgs->GetIntermediatePoints(&pointerPoints);

The recognizer needs the intermediate points even if the point hasn't moved. This is why AddPointerMoveDataToRecognizer(aArgs) was called before the HasPointMoved check. I've confirmed this work regresses bug 918937. We'll have to find a way around that.

Now that we have native touch injection apis we could write a long-tap test. hint hint, nudge nudge. :P

@@ -1357,3 @@
>    MOZ_ASSERT(event);
>  
> -  AutoDeleteEvent wrap(event);

Please remove the implementation of AutoDeleteEvent as well.
Attachment #8386510 - Flags: review?(jmathies) → review-
Posted patch Patch v2Splinter Review
(In reply to Jim Mathies [:jimm] from comment #19)
> Comment on attachment 8386510 [details] [diff] [review]
> Patch v1
> ::: widget/windows/winrt/MetroInput.cpp
> @@ +656,5 @@
> >      new WidgetTouchEvent(true, NS_TOUCH_MOVE, mWidget.Get());
> >    InitTouchEventTouchList(touchEvent);
> > +
> > +  Foundation::Collections::IVector<UI::Input::PointerPoint*>* pointerPoints;
> > +  aArgs->GetIntermediatePoints(&pointerPoints);
> 
> The recognizer needs the intermediate points even if the point hasn't moved.
> This is why AddPointerMoveDataToRecognizer(aArgs) was called before the
> HasPointMoved check. I've confirmed this work regresses bug 918937. We'll
> have to find a way around that.
>

Thanks for catching this! I'm now sending !HasPointMoved the touch move event to the gesture recognizer asynchronously, to make sure the event order is kept

> Now that we have native touch injection apis we could write a long-tap test.
> hint hint, nudge nudge. :P
> 

Couldn't find a bug, so created bug 981212 to track this.

> @@ -1357,3 @@
> >    MOZ_ASSERT(event);
> >  
> > -  AutoDeleteEvent wrap(event);
> 
> Please remove the implementation of AutoDeleteEvent as well.

Done.
Attachment #8386510 - Attachment is obsolete: true
Attachment #8387978 - Flags: review?(jmathies)

Updated

5 years ago
Attachment #8387978 - Flags: review?(jmathies) → review+
This does not need to be tracked for fx29 anymore since metro project was cancelled.
no longer blocking release as per comment 23
Assignee: rsilveira → nobody
We never shipped the metro support, closing!
Status: ASSIGNED → RESOLVED
Closed: 6 years agoLast year
Resolution: --- → WONTFIX

Updated

3 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.