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

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Event Handling
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Mehmet, Assigned: masayuki)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9.1 -
wanted1.9.1 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 15 obsolete attachments)

83.48 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.

Comment 1

10 years ago
-> 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

Updated

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

Comment 3

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

Comment 5

10 years ago
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

(Reporter)

Comment 6

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

Comment 8

10 years ago
Thank you very much !!!

Have a nice day !!!
(Reporter)

Comment 9

9 years ago
Hi Steven.

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

Greetings
Mehmet
(Reporter)

Comment 10

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

Comment 11

9 years ago
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

Updated

9 years ago
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?
I don't recall.
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.
Created attachment 348937 [details] [diff] [review]
Patch v1.0

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
Created attachment 348941 [details] [diff] [review]
Patch v1.0.1

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.
roc:

Do you remember it? Looks like we don't test mouse scroll excepting on XUL tree.
http://mxr.mozilla.org/mozilla-central/ident?i=sendMouseScrollEvent
http://mxr.mozilla.org/mozilla-central/ident?i=synthesizeMouseScroll
That's it.
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).
Created attachment 354580 [details] [diff] [review]
Patch v1.0.1 + tests

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?
Yeah, something like that
Created attachment 358630 [details] [diff] [review]
Patch v1.1

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...
Created attachment 358648 [details] [diff] [review]
Patch v1.1.1

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.
Created attachment 358687 [details] [diff] [review]
Patch v1.2

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)
Created attachment 358688 [details] [diff] [review]
Patch v1.2.1

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-
Created attachment 358842 [details] [diff] [review]
Patch v1.3

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.
Created attachment 359068 [details] [diff] [review]
Patch v1.4

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);
}
Created attachment 359276 [details] [diff] [review]
Patch v1.4.1

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.
Created attachment 361032 [details] [diff] [review]
Patch v1.4.2
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.
Created attachment 361259 [details] [diff] [review]
Patch v1.4.3

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)
Created attachment 361260 [details] [diff] [review]
Patch v1.4.4

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).
Created attachment 361473 [details] [diff] [review]
Patch v1.4.5

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
Last Resolved: 9 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.

Comment 69

9 years ago
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.
Created attachment 361800 [details] [diff] [review]
Patch v1.4.6

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)
Created attachment 361811 [details] [diff] [review]
Patch v1.4.7

see comment 70 for the detail.
Attachment #361800 - Attachment is obsolete: true
Attachment #361811 - Flags: review?(roc)
Attachment #361811 - Flags: review?(roc) → review+
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
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Depends on: 478536
Depends on: 485994
Depends on: 491712
Depends on: 502384
You need to log in before you can comment on or make changes to this bug.