Closed Bug 953012 Opened 10 years ago Closed 6 years ago

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

Categories

(Core Graveyard :: Widget: WinRT, defect, P1)

x86_64
Windows 8.1
defect

Tracking

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

RESOLVED WONTFIX
Tracking Status
firefox27 --- unaffected
firefox28 - affected
firefox29 - affected
firefox30 - affected

People

(Reporter: kjozwiak, Unassigned)

References

Details

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

Attachments

(2 files, 4 obsolete files)

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
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: 10 years ago
Resolution: --- → WORKSFORME
For iteration #22, verified as fixed with latest Nightly on Win 8.1 64-bit.
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.
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
Attached 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
Attached 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
Attached patch WIP (obsolete) — Splinter Review
Forgot to qref.
Attachment #8386229 - Attachment is obsolete: true
Attached 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+
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-
Attached 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)
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: 10 years ago6 years ago
Resolution: --- → WONTFIX
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: