Closed Bug 442774 Opened 12 years ago Closed 11 years ago

Wheel/touchpad scrolling gets stuck in frame, stop scrolling the web page as a whole

Categories

(Core :: DOM: UI Events & Focus Handling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mehmet.sahin, Assigned: masayuki)

References

()

Details

Attachments

(1 file, 15 obsolete files)

83.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.1pre) Gecko/2008062922 Camino/2.0a1pre (like Firefox/3.0.1pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.1pre) Gecko/2008062922 Camino/2.0a1pre (like Firefox/3.0.1pre)

When I move the mousecursor in CAMINO 2.0a1pre to a frame with a scrollbar (in the sample-url to the frame with the pictures, where you can select the
youtube-videos), and scroll with the touchpad of a macbook
(two-finger-scrolling) in this frame, the cursor will hang only in this frame
with the scrollbar, without to scroll forward through the whole webpage when
the end of the frame is reached. 

Scrolling forward the page is then only possible when I move the cursor to the
outside of the frame.

(In CAMINO 1.6.1 this Problem does not exist!!!)

(Same problem with Firefox 3)

Hope I could explain the Problem - it reduce the performance of the touchpad.

Thank you in advance.

Reproducible: Always

Steps to Reproduce:
1. Move the cursor into a frame with a scrollbar.
2. Now scroll with the touchpad of a macbook (two-finger-scrolling) or with the
mousewheel (without to move the mouse) within this frame.
3.
Actual Results:  
The cursor hangs only in the frame with the scrollbar, without to scroll
forward through the whole webpage.

Expected Results:  
Scrolling forward through page when the end of the frame with the scrollbar is
reached without to hang in the frame.
-> Cocoa widgets for triage, given that it's a cross-product widgets issue.
Assignee: nobody → joshmoz
Severity: major → normal
Component: General → Widget: Cocoa
Product: Camino → Core
QA Contact: general → cocoa
Version: unspecified → Trunk
Summary: problem with the touchpad of macbook while scrolling through a webpage → Wheel/touchpad scrolling gets stuck in frame, stop scrolling the web page as a whole
Mehmet, your description isn't very clear.  But I guess what you're
reporting is this:

In Firefox 2 (and Camino 1.6), if you scroll with the mouse wheel over
a div with scrollable contents, the whole page will scroll up after
you've scrolled to the top of the div, and the whole page will scroll
down after you've scrolled to the bottom of the div.  This is true
even while the mouse is over the div (with scrollable contents).

But this is no longer true in Firefox 3 (or 1.9.0-branch Camino).  To
scroll the page you always have to move the mouse outside the div with
scrollable contents.

I don't know whether this is a bug or a feature :-)  And in any case
it happens on both OS X and Windows, so its cross-platform.

I've no idea how to classify this.

Stuart, any ideas?
Status: UNCONFIRMED → NEW
Component: Widget: Cocoa → General
Ever confirmed: true
OS: Mac OS X → All
Hardware: PC → All
Assignee: joshmoz → nobody
QA Contact: cocoa → general
Hello Steven, hello Stuart,

thank you for your fast answers.

I am sorry for my bad description of the problem, but my english is very, very bad ;-)


Steven, you are exactly right with summary of my problem !!!

For me the user of Camino 2 or Firefox 3, this problem is definitely a bug. Why it should be a feature :-) , because this problem reduce the performance of the touchpad ;-(

Hope this problem will be fixed, because I love Camino 2 / Firefox 3 and to work with the touchpad of a macbook ;-)


THANK YOU FOR YOUR HELP !!!!!!!

Greetings from Germany

Mehmet
You're most welcome, Mehmet.

> For me the user of Camino 2 or Firefox 3, this problem is definitely
> a bug. Why it should be a feature :-)

The FF3 developers may have deliberately dropped this FF2 feature.

UI design is a tricky business ... and sometimes (I think) rather
arbitrary :-)

But I certainly don't know.  I'm going to recategorize this bug to
where it may receive more attention -- though I'm still not entirely
sure where it should go.
Assignee: nobody → jag
Component: General → XP Toolkit/Widgets
QA Contact: general → xptoolkit.widgets
Hi  Steven,

okay, I understand what you mean...

I am confident of that the recategorizing of the bug will help to find a solution ;-)   STEP BY STEP ;-)


Thank you very much for your help !!!


Best regards
Mehmet

Hello Steven,

I have heard nothing since the recategorizing of the bug   :-(

Have you some news perhaps ?

Greetings
Mehmet
I'll try recategorizing this again.
Assignee: jag → nobody
Component: XP Toolkit/Widgets → Layout
QA Contact: xptoolkit.widgets → layout
Thank you very much !!!

Have a nice day !!!
Hi Steven.

Again I heard nothing since 3 weeks ?!?!?!  ;-(

Greetings
Mehmet
Hello Bugzilla-Team:

i would like to ask what is about this bug ?

there was no reaction since 2008-07-17 to this bug ;-(

Thank you for your help in advance.

Greetings
Mehmet
Hello Steven,

there is nothing happen since your last categorizing.

No Reaction, No answers ;-(

Could do something, that this bug receive more attention...

Thank you in advance.

Mehmet
Flags: wanted1.9.1?
If you keep adding comments to the bug, it just becomes longer and thus harder to read and understand, which makes it less likely to be fixed.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1-
Roc, do you know why this change was made from FF2 to FF3 ... or whether it was made on purpose?
The behavior change is a feature. See bug 312831.

However, I think that we can improve the behavior for non-mouse users. Current design is that first wheel scrolling event creates a transaction for current scrollable view which is under the mouse cursor. The transaction will be free after 1.5 sec from last wheel event. But in this case, looks like the time is too long for the reporter (and maybe, for all touch pad users). And if users want to scroll to non-scrollable direction in the current scrollable view at many times, we should discard the current transaction.
Attached patch Patch v1.0 (obsolete) — Splinter Review
Maybe, we should not update the transaction when the mouse scroll event cannot scroll the current view actually. Then, the transaction will be finished by timeout.

So, with default settings, the scroll events are discarded during 1.5 sec since last scrolled event.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #348937 - Flags: superreview?(roc)
Attachment #348937 - Flags: review?(roc)
The patch also fixes bug 377404.
Blocks: 377404
Component: Layout → Event Handling
QA Contact: layout → events
Attached patch Patch v1.0.1 (obsolete) — Splinter Review
Sorry, the previous patch has unneeded code moving.
Attachment #348937 - Attachment is obsolete: true
Attachment #348941 - Flags: superreview?(roc)
Attachment #348941 - Flags: review?(roc)
Attachment #348937 - Flags: superreview?(roc)
Attachment #348937 - Flags: review?(roc)
The patch looks good, but can we have some tests for this functionality?
I wrote a mochitest a while ago that tests mouse-scroll events, although I can't now remember what the filename is.
ok, but looks like we should add a new file for wheel scroll tests.
>     var prevPos = root.scrollTop;
>     synthesizeMouseScroll(...
>     isnot(prevPos, root.scrollTop, "failed to scroll (vertical/backward)");

Ugh, scrollTop returns same value before calling synthesizeMouseScroll. If I call alert after synthesizeMouseScroll, then, scrollTop returns scrolled value. How do sync the scrolling?
mousescroll is always asynchronous.
You can capture "scroll" event, which is dispatched
after the scrolling actually happened.
Thank you, Smaug. I'll try to create the new tests.
Uh, the asynchronous scrolling is too difficult for continuous event firing. |nsNSElementTearoff::GetScrollTop| calls |nsScrollPortView::GetScrollPosition| which returns actual scroll position. But I hope that |nsNSElementTearoff::GetScrollTop| returns the destination positions of the view. Can I change |nsNSElementTearoff::GetScroll*| behavior as so? (Of course, I don't change |nsScrollPortView::GetScrollPosition|. I mean I'll create new method to nsScrollPortView.)

Looks like roc's tree scrolling tests uses destination position:
http://mxr.mozilla.org/mozilla-central/source/toolkit/content/tests/widgets/tree_shared.js#1105
Don't change GetScroll*.

I guess that test works because tree scrolling is synchronous.

I would listen for the "scroll" event. When that fires, scrollTop should have the right value. Although probably, to account for possible smooth scrolling, you should wait until you get a "scroll" event with the right scrollTop value, since there might be intermediate "scroll" events with intermediate scrollTop values.
Comment on attachment 348941 [details] [diff] [review]
Patch v1.0.1

I'll post the tests.

Roc, the tests are too delicate. The tests are failed by mouse move events of real. Can I suppress the native mouse move events during the tests? Or, should I add new way for suppressing the events?

E.g., nsDOMWindowUtils adds a new flag to nsEvent. When a wheel transaction begins with an event which has the flag, the transaction don't finish by native mouse events.
Attachment #348941 - Flags: superreview?(roc)
Attachment #348941 - Flags: review?(roc)
I would prefer to add an API to nsDOMWindowUtils which simply disables all mouse events for the presshell (throwing them away).
Attached patch Patch v1.0.1 + tests (obsolete) — Splinter Review
Thank you, roc. Please look the comments in the tests for the detail.
Attachment #348941 - Attachment is obsolete: true
Attachment #354580 - Flags: superreview?(roc)
Attachment #354580 - Flags: review?(roc)
+function stopMouseEventHandling(aStop, aWindow)
+{
+  netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
+
+  if (!aWindow)
+    aWindow = window;
+
+  var utils =
+    aWindow.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
+            getInterface(Components.interfaces.nsIDOMWindowUtils);
+  if (utils)
+    utils.stopMouseEventHandling(aStop);
+}

What's the point of having an aWindow parameter since it doesn't matter what window is used, anyway?

+function testCoutinuousScroll()

testContinuousScroll

These timers are going to cause problems with random test failures due to missing timeouts in very slow test machines. I think we'll be OK if we set kTimeout and kIgnoreMoveDelay to very large values, make testCoutinuousScroll and testOneTimeScroll listen for an onscroll event instead of using setTimeout to try to guess when the scroll happened. I guess we just can't test testRestartScroll reliably and should take that out.

+  // Send a mouse move event immediately after last wheel event, then, current
+  // transaction should be kept.
+  { func: sendMouseMoveEvent, delay: kIgnoreMoveDelay / 3,
+    offset: kPtInSubView1ForV },

Can't delay be 0 here? We really need to get rid of all 'delay's.

+  // Send a wheel event after |kIgnoreMoveDelay| milliseconds since last mouse
+  // move event but it is fired immediately after the last wheel event, then,
+  // current transaction should be kept.
+  { func: sendMouseMoveEvent, delay: 0, offset: kPtInSubView1ForV },
+  { func: testOneTimeScroll, delay: kEnoughForIgnoreMoveDelay,
+    offset: kPtInSubView1ForV,
+    isForward: true, isVertical: true, expectedView: gRootView,
+    description: "Reset transaction by a mouse move event (v-7)" },

This test will probably fail randomly. Similarly for all the other tests that have non-zero delays. The only way I can think of to make reliable tests here would be to fire a special DOM event when the internal mouse-scroll timers fire, and listen for that event instead of using a timeout.
> to fire a special DOM event when the internal mouse-scroll timers fire,

I don't understand this well. What's the special DOM event?

Did you mean that ESM should fire a special DOM event, e.g., "on timeout wheel scroll transaction" event and if a test got the event before expected scroll event, then, retry the tests?
Attached patch Patch v1.1 (obsolete) — Splinter Review
I added MozMouseScrollFailed event and MozMouseScrollTransactionTimeout event for testing. MozMouseScrollFailed event is fired when the scroll target cannot be scroll to the direction. MozMouseScrollTransactionTimeout event is fired when the transaction is timed out.

But the timeout event is sometimes fired after the new scroll event. I'm not sure the reason. (nsEventDispatcher::Dispatch doesn't fire the event immediately?)

And I changed the nsIPresShell::StopMouseEventHandling to nsIPresShell::StopNonSynthesizedMouseEventHandling. Because when I synthesize some mouse events, I need to restart the mouse event handling temporally, but at that time, real mouse events break the testing (especially, mouse move events).

And when the test gets unexpected timeout event, it is retest current test-list no more than 5 times. This should suppress random test failures.
Attachment #354580 - Attachment is obsolete: true
Attachment #358630 - Flags: superreview?(roc)
Attachment #358630 - Flags: review?(roc)
Attachment #354580 - Flags: superreview?(roc)
Attachment #354580 - Flags: review?(roc)
Comment on attachment 358630 [details] [diff] [review]
Patch v1.1

Ah, the patch might have a mistake. Please wait...
Attachment #358630 - Flags: superreview?(roc)
Attachment #358630 - Flags: review?(roc)
Attachment #358630 - Flags: superreview?(roc)
Attachment #358630 - Flags: review?(roc)
Comment on attachment 358630 [details] [diff] [review]
Patch v1.1

sorry for the spam. I tested with:

> nsMouseWheelTransaction::OnEvent(nsEvent* aEvent)
> {
>   if (!sTargetFrame)
>     return;
> 
>   if (OutOfTime(sTime, GetTimeoutTime())) {
>     // Even if the scroll event which is handled after timeout, but onTimeout
>     // was not fired by timer, then the scroll event will scroll old frame,
>     // therefore, we should call OnTimeout here and ensure to finish the old
>     // transaction.
>     OnTimeout(nsnull, nsnull);
>     return;
>   }
> 

But the new scroll event still comes before timeout event...
Attached patch Patch v1.1.1 (obsolete) — Splinter Review
sorry, previous patch has uncompleted comment.
Attachment #358630 - Attachment is obsolete: true
Attachment #358648 - Flags: superreview?(roc)
Attachment #358648 - Flags: review?(roc)
Attachment #358630 - Flags: superreview?(roc)
Attachment #358630 - Flags: review?(roc)
I don't know why you need the MozMouseScrollFailed event.

> But the timeout event is sometimes fired after the new scroll event.

What do you mean exactly?

> (nsEventDispatcher::Dispatch doesn't fire the event immediately?)

It does.
(In reply to comment #39)
> I don't know why you need the MozMouseScrollFailed event.

It helps to create this test:
1. I need to call one more setTimeout for checking the failure of scrolling. And I need to clear the timer if I don't need it by other events. Of course, a timeout event and the failure event order might be reverted by slow testing environment without recoding the each updating transaction time ourselves.
2. If the failure event came before scroll event by slow testing environment, it might create complex failure log.

I hope to remove the random possibility which is by timer.

> > But the timeout event is sometimes fired after the new scroll event.
> 
> What do you mean exactly?

In testRestartScroll, I expected to test following steps:
1. firing wheel events until timeout event coming.
2. checking transaction timeout (set wasTransactionTimeout flag in the event)
3. firing a wheel event
4. checking scroll event on expected view.

However, in the expected scroll event, the wasTransactionTimeout event was not set.

> > (nsEventDispatcher::Dispatch doesn't fire the event immediately?)
> 
> It does.

Ugh... in that case, my test had a bug in above test (wasTransactionTimeout)... I'll retry to check it.
Attached patch Patch v1.2 (obsolete) — Splinter Review
Oh, it's strange. I rewrote restartScrollTest. Then, the test passed easily :-(
My old logic had bug, but the old code is not there, unfortunately...
Sorry for the spam.
Attachment #358648 - Attachment is obsolete: true
Attachment #358687 - Flags: superreview?(roc)
Attachment #358687 - Flags: review?(roc)
Attachment #358648 - Flags: superreview?(roc)
Attachment #358648 - Flags: review?(roc)
Attached patch Patch v1.2.1 (obsolete) — Splinter Review
oops, I forget to remove the commented out code, sorry.
Attachment #358687 - Attachment is obsolete: true
Attachment #358688 - Flags: superreview?(roc)
Attachment #358688 - Flags: review?(roc)
Attachment #358687 - Flags: superreview?(roc)
Attachment #358687 - Flags: review?(roc)
(In reply to comment #40)
> 2. checking transaction timeout (set wasTransactionTimeout flag in the event)

s/wasTransactionTimeout/testRestartScroll

> However, in the expected scroll event, the wasTransactionTimeout event was not
> set.

s/wasTransactionTimeout event/onTransactionTimeout event

> Ugh... in that case, my test had a bug in above test (wasTransactionTimeout)...

s/wasTransactionTimeout/testRestartScroll

(In reply to comment #41)
> Oh, it's strange. I rewrote restartScrollTest. Then, the test passed easily :-(

s/restartScrollTest/testRestartScroll
I think instead of defining full-fledged DOM events here, you can use generic "user-defined" nsDOMEvents. Dispatch an nsDOMEvent with NS_USER_DEFINED_EVENT in the nsEvent and then call nsEventDispatcher::CreateEvent and DispatchDOMEvent.

Otherwise looks reasonable.
Comment on attachment 358688 [details] [diff] [review]
Patch v1.2.1

>+  nsEvent event(PR_TRUE, NS_MOUSE_SCROLL_FAILED);
>+  event.time = PR_IntervalNow();
>+  nsEventStatus status = nsEventStatus_eIgnore;
>+  nsIFrame* frame = sTargetFrame;
>+  nsEventDispatcher::Dispatch(sTargetFrame->GetContent(),
>+                              sTargetFrame->PresContext(),
>+                              &event, nsnull, &status);

The easiest way to dispatch a simple DOM event is probably to use
nsContentUtils::DispatchTrustedEvent. And no need to add support for onFoo attributes...

nsContentUtils::DispatchTrustedEvent(sTargetFrame->GetContent()->GetOwnerDoc(),
                                     sTargetFrame->GetContent(),
                                     NS_LITERAL_STRING("MozMouseScrollFailed"),
                                     PR_TRUE, PR_TRUE)

>+<body xmlns="http://www.w3.org/1999/xhtml"
>+      onMozMouseScrollFailed="onMouseScrollFailed(event);"
>+      onMozMouseScrollTransactionTimeout="onTransactionTimeout();">
...of course then you need to use .addEventListener() to add the listener.
Attachment #358688 - Flags: review?(roc) → review-
Attached patch Patch v1.3 (obsolete) — Splinter Review
Thank you, Smaug and roc!
Attachment #358688 - Attachment is obsolete: true
Attachment #358842 - Flags: superreview?(roc)
Attachment #358842 - Flags: review?(Olli.Pettay)
Attachment #358688 - Flags: superreview?(roc)
Comment on attachment 358842 [details] [diff] [review]
Patch v1.3

>+nsCOMPtr<nsITimer> nsMouseWheelTransaction::sTimer;
One shouldn't use static nsCOMPtr<>s.
on IRC: "bsmedberg: ...forgetting to release one usually crashes on shutdown, or at least causes very bad things"

>+void
>+nsMouseWheelTransaction::SetTimeout()
>+{
>+  if (!sTimer)
>+    sTimer = do_CreateInstance(NS_TIMER_CONTRACTID);
>+  else
>+    sTimer->Cancel();
>+  NS_ASSERTION(sTimer, "sTimer is null");
This is wrong. sTimer can be null when OOM.
So don't assert but return.

>+
>+  /**
>+   * Stop or restart non synthesized mouse event handling on *all* windows.
So what is a "synthesized mouse event" (NS_EVENT_FLAG_SYNTHESIZED) when comparing to a 
nsMouseEvent which has reasonType eSynthesized?
Please document the difference clearly at least in nsGUIEvent.h, 
and/or perhaps rename NS_EVENT_FLAG_SYNTHESIZED.
And why NS_EVENT_FLAG_SYNTHESIZED flag is set also to key events and gesture events if 
it is never used with those events?

>+   *
>+   * Cannot be accessed from unprivileged context (not content-accessible)
Missing '.' after ')'

(I don't like the extra events too much, but if there really isn't any other way to
write tests, I guess they are ok.)
Attachment #358842 - Flags: review?(Olli.Pettay) → review-
(In reply to comment #47)
> (From update of attachment 358842 [details] [diff] [review])
> >+nsCOMPtr<nsITimer> nsMouseWheelTransaction::sTimer;
> One shouldn't use static nsCOMPtr<>s.
> on IRC: "bsmedberg: ...forgetting to release one usually crashes on shutdown,
> or at least causes very bad things"

Thank you for your review.

O.K. What should do? I should not use nsCOMPtr at static variables? Or Should I guarantee it will be free before shoutdown? E.g., When nsEventStateManager::Shutdown is called, we can clear it too.
I guess using nsITimer* and manually AddRef/Release.
Release in nsEventStateManager::~nsEventStateManager() when sESMInstanceCount
drops to 0.
Attached patch Patch v1.4 (obsolete) — Splinter Review
I renamed the flag to NS_EVENT_FLAG_EMULATION. And also I renamed the related functions to stopNonEmulatedMouseEventHandling.

> +  if (!sTimer) {
> +    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
> +    if (!timer)
> +      return;
> +    sTimer = timer;
> +    NS_ADDREF(sTimer);
> +  }

Looks ugly... Do you have better idea for this code?
Attachment #358842 - Attachment is obsolete: true
Attachment #359068 - Flags: review?(Olli.Pettay)
Attachment #358842 - Flags: superreview?(roc)
You could make it
if (!sTimer) {
  nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
  if (!timer)
    return;
  timer.swap(sTimer);
}
Attached patch Patch v1.4.1 (obsolete) — Splinter Review
Thank you, Smaug, this uses |.swap()|.
Attachment #359068 - Attachment is obsolete: true
Attachment #359276 - Flags: review?(Olli.Pettay)
Attachment #359068 - Flags: review?(Olli.Pettay)
Smaug? Would you review the latest patch?
Comment on attachment 359276 [details] [diff] [review]
Patch v1.4.1

(As I said, I really don't like those new events, but if that is the only way to make the functionality testable...)

>+nsMouseWheelTransaction::OnFailToScrollTarget()
>+{
>+  NS_PRECONDITION(sTargetFrame, "We don't have mouse scrolling transaction");
>+  nsContentUtils::DispatchTrustedEvent(
>+                    sTargetFrame->GetContent()->GetOwnerDoc(),
>+                    sTargetFrame->GetContent(),
>+                    NS_LITERAL_STRING("MozMouseScrollFailed"),
>+                    PR_TRUE, PR_TRUE);
Please add some comment here why the event is dispatched, and perhaps
this bug number too.
;
>+  nsContentUtils::DispatchTrustedEvent(
>+                    frame->GetContent()->GetOwnerDoc(),
>+                    frame->GetContent(),
>+                    NS_LITERAL_STRING("MozMouseScrollTransactionTimeout"),
>+                    PR_TRUE, PR_TRUE);
Same here.

>+void
>+nsMouseWheelTransaction::SetTimeout()
>+{
>+  if (!sTimer) {
>+    nsCOMPtr<nsITimer> timer = do_CreateInstance(NS_TIMER_CONTRACTID);
>+    if (!timer)
>+      return;
>+    timer.swap(sTimer);
>+  }
>+  sTimer->Cancel();
>+  nsresult rv =
>+    sTimer->InitWithFuncCallback(OnTimeout, nsnull, GetTimeoutTime(),
>+                                 nsITimer::TYPE_ONE_SHOT);
>+  NS_ASSERTION(NS_SUCCEEDED(rv), "nsITimer::InitWithFuncCallback failed");
This shouldn't be an assertion. InitWithFuncCallback may fail for example when OOM.
You could use NS_WARN_IF_FALSE(..., ...).

>+// XXX Where should I change the code by this comment??
I don't understand this comment.

>+// When an event is synthesized for testing, this flag will be set.
>+#define NS_EVENT_FLAG_EMULATION           0x1000
Add a comment that this is currently used only for mouse events.
Attachment #359276 - Flags: review?(Olli.Pettay) → review+
(In reply to comment #54)
> (From update of attachment 359276 [details] [diff] [review])
> (As I said, I really don't like those new events, but if that is the only way
> to make the functionality testable...)

The event adding for tests is roc's suggestion, see comment 32. Because if I don't use them, the tests may fail randomly. And the events helps to make simple testing code.

> >+// XXX Where should I change the code by this comment??
> I don't understand this comment.

Ah, I misunderstood the comment. I was thinking that I need to add same flag to another point. I'll just remove this comment.
Attached patch Patch v1.4.2 (obsolete) — Splinter Review
Attachment #359276 - Attachment is obsolete: true
Attachment #361032 - Flags: superreview?(roc)
In some places in these tests, we're waiting for 10 seconds just for one step, right? It seems that these tests are going to be slow.

Instead of "emulated" events, I think we should describe them as synthesized events.
(In reply to comment #57)
> In some places in these tests, we're waiting for 10 seconds just for one step,
> right? It seems that these tests are going to be slow.

Yes. But we can reduce the value at first trying. And when we retry to the tests, we can grow up the value.

> Instead of "emulated" events, I think we should describe them as synthesized
> events.

"synthesized" is already used for event reason. Probably it is used for mouse move event synthesizing at scrolling. Smaug was confused by it. Do you have better idea for this issue?
(In reply to comment #58)
> (In reply to comment #57)
> > In some places in these tests, we're waiting for 10 seconds just for one step,
> > right? It seems that these tests are going to be slow.
> 
> Yes. But we can reduce the value at first trying. And when we retry to the
> tests, we can grow up the value.

OK, but we don't do that yet, right?

> > Instead of "emulated" events, I think we should describe them as synthesized
> > events.
> 
> "synthesized" is already used for event reason. Probably it is used for mouse
> move event synthesizing at scrolling. Smaug was confused by it. Do you have
> better idea for this issue?

Those are synthesized events generated at reflow/scrolling. Maybe call them "synthetic test events"? They're generated by methods that have "synthesize" in the name.
Attached patch Patch v1.4.3 (obsolete) — Splinter Review
This fixes the two issue. At first time, we test with default pref settings. Otherwise, using 5000/1000ms.

And also, I found a bug of the retry testing system. This patch clear a pending timer when it retries to a test. Without this fix, the next or later testing gets the previous testing action.
Attachment #361032 - Attachment is obsolete: true
Attachment #361259 - Flags: superreview?(roc)
Attachment #361032 - Flags: superreview?(roc)
Attached patch Patch v1.4.4 (obsolete) — Splinter Review
sorry for the spam.
Attachment #361259 - Attachment is obsolete: true
Attachment #361260 - Flags: superreview?(roc)
Attachment #361259 - Flags: superreview?(roc)
+  void stopNonSyntheticTestMouseEventHandling(in boolean aStop);

that's uglier than I expected.

How about stopNonTestMouseEventHandling? Or disableNonTestMouseEvents? I prefer the latter.

How long do the tests take to run now?
(In reply to comment #62)
> How long do the tests take to run now?

about 15sec on my environment (Phenom 9500/Vista x64).
Attached patch Patch v1.4.5 (obsolete) — Splinter Review
renamed.
Attachment #361260 - Attachment is obsolete: true
Attachment #361473 - Flags: superreview?(roc)
Attachment #361260 - Flags: superreview?(roc)
Attachment #361473 - Flags: superreview?(roc) → superreview+
pushed to trunk.
http://hg.mozilla.org/mozilla-central/rev/2e550e7b8f3c
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
backed-out. the patch might be a cause of timeout of test_keycodes.xul. I want
to watch the tinderbox state without this patch.

simplest failure log:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1234280239.1234287493.31852.gz
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This is the cause. But I have no idea what is a problem.
I can reproduce the failure with all chrome test running.

Fx crash in nsXBLService::GetBinding(nsIContent* aBoundElement, nsIURI* aURI, 
                         PRBool aPeekOnly, nsIPrincipal* aOriginPrincipal,
                         PRBool* aIsReady, nsXBLBinding** aResult,
                         nsTArray<nsIURI*>& aDontExtendURIs)

At this time, aURI was already freed (the debugger displayed 0xdddddddd for the value). At this time, it is called from:

nsXBLService::GetBinding(nsIContent* aBoundElement, nsIURI* aURI, 
                         PRBool aPeekOnly, nsIPrincipal* aOriginPrincipal,
                         PRBool* aIsReady, nsXBLBinding** aResult,
                         nsTArray<nsIURI*>& aDontExtendURIs)

here:

    // Use the NodePrincipal() of the <binding> element in question
    // for the security check.
    rv = GetBinding(aBoundElement, baseProto->BindingURI(), aPeekOnly,
                    child->NodePrincipal(), aIsReady,
                    getter_AddRefs(baseBinding), aDontExtendURIs);

They are called for autocomplete popup invalidating from nsAutoCompleteController::ProcessResult. This is called for searching the autocomplete candidate list. It is called from timer of nsAutoCompleteController.

I don't understand what is a problem.
historically the autocomplete stuff doesn't understand that it can reenter some portion of its class (event handling is fun). in general the fix for such a thing is to keep a local variable or null check the member variable (depending on which behavior you want) across the methods which can call out.
Attached patch Patch v1.4.6 (obsolete) — Splinter Review
I give up to fix the crash bug, it should not be the scope of this bug. The patch escapes from the crash.

> diff --git a/widget/tests/test_keycodes.xul b/widget/tests/test_keycodes.xul
> --- a/widget/tests/test_keycodes.xul
> +++ b/widget/tests/test_keycodes.xul
> @@ -696,16 +696,20 @@ function runTextInputTests()
>              "0");
>      testKey({layout:"Lithuanian", keyCode:56, ctrl:1, alt:1, chars:"8"},
>              "8");
>      testKey({layout:"Lithuanian", keyCode:57, ctrl:1, alt:1, chars:"9"},
>              "9");
>      testKey({layout:"Lithuanian", keyCode:48, ctrl:1, alt:1, chars:"0"},
>              "0");
>    }
> +
> +  // XXX We need to move focus for canceling to search the autocomplete
> +  // result. If we don't do here, Fx will crash at end of this tests.
> +  document.getElementById("button").focus();
>  }
>  
>  function runTest()
>  {
>    runKeyEventTests();
>    runAccessKeyTests();
>    runXULKeyTests();
>    runTextInputTests();

I guess that the autocomplete result searching is still processed by timer even after closing the test page. And it is a cause of the crash. Therefore, if I move the focus from textbox before finish the test, the crash is not occurred.

If I remove the new test, the crash is not occurred. However, if I add the test to before/after test_keycodes.xul, the crash is always occurred. I'm not sure why the new test is a trigger of the bug.
Attachment #361473 - Attachment is obsolete: true
Attachment #361800 - Flags: review?(roc)
Comment on attachment 361800 [details] [diff] [review]
Patch v1.4.6

oops, sorry for the spam. the patch has unexpected change.
Attachment #361800 - Flags: review?(roc)
Attached patch Patch v1.4.7Splinter Review
see comment 70 for the detail.
Attachment #361800 - Attachment is obsolete: true
Attachment #361811 - Flags: review?(roc)
sigh...

I relanded the patch, but two crash test is failed on Linux and Windows (one is Linux, other is Windows). Does my patch breaks memory??
Ugh, the both crash tests are passed in next cycle which contains the patch (i.e., not backed-out)...
ok, the both failures are not caused my patch. I found them in 2/11 and 2/8's tinderbox log.
relanded.
http://hg.mozilla.org/mozilla-central/rev/e0ff38f1faa1
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Depends on: 485994
Depends on: 491712
Depends on: 502384
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.