[e10s] _setupSwipeGesture() tries to access content when scrolling down

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 35
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

.... and throws "TypeError: content is null".
Blocks: 673875
Seized the chance to add a proper uninit() function. I would have not added listeners at all but the comment at the top of the file seems to indicate that we want to cancel events.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Attachment #8489135 - Flags: review?(felipc)
Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch

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

::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> +      return;
> +    }
> +
> +    for (let name of this.GESTURE_EVENTS) {
> +      addEventListener(`Moz${name}`, this, true);

What's this?
Attachment #8489135 - Flags: review?(felipc) → review+
Does this mean that we won't/can't support swipe animations in e10s?
Comment on attachment 8489135 [details] [diff] [review]
0001-Bug-1067128-_setupSwipeGesture-tries-to-access-conte.patch

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

Sorry, I have no idea what happened.

::: browser/base/content/browser-gestureSupport.js
@@ +44,5 @@
> +      return;
> +    }
> +
> +    for (let name of this.GESTURE_EVENTS) {
> +      addEventListener(`Moz${name}`, this, true);

Oh, http://tc39wiki.calculist.org/es6/template-strings/ I guess.
Attachment #8489135 - Flags: review+ → review?(felipc)
(In reply to Josh Matthews [:jdm] from comment #2)
> > +    for (let name of this.GESTURE_EVENTS) {
> > +      addEventListener(`Moz${name}`, this, true);

Yes, those are template strings :)

(In reply to Stephen Pohl [:spohl] from comment #3)
> Does this mean that we won't/can't support swipe animations in e10s?

This means we need to rewrite it to support e10s.
> (In reply to Stephen Pohl [:spohl] from comment #3)
> > Does this mean that we won't/can't support swipe animations in e10s?
> 
> This means we need to rewrite it to support e10s.

I'm assuming that we're going with this workaround because e10s is on a tight deadline? Is there already a bug on file to implement this for e10s? IIRC this also affects history swipe navigation without animations that some people have come to rely on.
Yeah, we're going with lots of workarounds because we want to make basic browsing in e10s work while iteratively fixing all the stuff that hasn't been written with e10s in mind.

Bug 863514 was filed to fix this. Are you talking about the back/fwd navigation with two-finger swipes? I of course don't want that to break and I think I tested that with the given patch applied. I really just want to get rid of that error for now when scrolling.
(In reply to Tim Taubert [:ttaubert] from comment #7)
> Bug 863514 was filed to fix this.

Thanks! I wasn't aware of this bug but will make sure to CC myself.

> Are you talking about the back/fwd
> navigation with two-finger swipes? I of course don't want that to break and
> I think I tested that with the given patch applied. I really just want to
> get rid of that error for now when scrolling.

Yes, I was expecting back/forward navigation with two-finger swipes to break with this patch and I still get this impression after quickly reading through browser-gestureSupport.js. It might be better to just leave isVerticalSwipe set to false when gMultiProcessBrowser is true in _setupSwipeGesture. If I'm mistaken and regular two-finger swipe navigation is indeed unbroken, please ignore my comment.
(In reply to Stephen Pohl [:spohl] from comment #8)
> It might be better to just leave isVerticalSwipe set to false when
> gMultiProcessBrowser is true in _setupSwipeGesture.

... and skip over the part that's accessing 'content', of course.
I would expect something like this to work:

diff --git a/browser/base/content/browser-gestureSupport.js b/browser/base/content/browser-gestureSupport.js
--- a/browser/base/content/browser-gestureSupport.js
+++ b/browser/base/content/browser-gestureSupport.js
@@ -184,22 +184,22 @@ let gGestureSupport = {
    */
   _setupSwipeGesture: function GS__setupSwipeGesture(aEvent) {
     if (!this._swipeNavigatesHistory(aEvent)) {
       return false;
     }
 
     let isVerticalSwipe = false;
     if (aEvent.direction == aEvent.DIRECTION_UP) {
-      if (content.pageYOffset > 0) {
+      if (gMultiProcessBrowser || content.pageYOffset > 0) {
         return false;
       }
       isVerticalSwipe = true;
     } else if (aEvent.direction == aEvent.DIRECTION_DOWN) {
-      if (content.pageYOffset < content.scrollMaxY) {
+      if (gMultiProcessBrowser || content.pageYOffset < content.scrollMaxY) {
         return false;
       }
       isVerticalSwipe = true;
     }
     if (isVerticalSwipe) {
       // Vertical overscroll has been temporarily disabled until bug 939480 is
       // fixed.
       return false;
Yeah that sounds like the right solution for now.
Attachment #8489135 - Attachment is obsolete: true
Attachment #8489135 - Flags: review?(felipc)
Attachment #8489588 - Flags: review?(spohl.mozilla.bugs)
Attachment #8489588 - Flags: review?(felipc)
Comment on attachment 8489588 [details] [diff] [review]
0001-Bug-1067128-e10s-_setupSwipeGesture-tries-to-access-.patch, v2

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

Thanks!
Attachment #8489588 - Flags: review?(spohl.mozilla.bugs) → review+
Attachment #8489588 - Flags: review?(felipc)
https://hg.mozilla.org/mozilla-central/rev/3dd054c861f5
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.