[e10s] Back/forward gesture causes "unsafe CPOW usage" warning

RESOLVED FIXED in Firefox 45

Status

()

Firefox
General
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mconley, Assigned: Neil Deakin)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 45
All
Mac OS X
Points:
---

Firefox Tracking Flags

(e10sm8+, firefox45 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

6.35 KB, text/plain
Felipe
: review+
Details
STR:

1) In a remote browser, do some browsing so that you have some session history
2) Use the "back" gesture to go back a page. On my MacBook, that's 3-finger swipe left, but it might be different for other folk.
3) Use the "forward" gesture to go forward to where you came from.

This causes some "unsafe CPOW usage" warnings in the Browser Console at the following locations:

in browser/base/content/browser.js:

  /**
   * Starts the swipe animation and handles fast swiping (i.e. a swipe animation
   * is already in progress when a new one is initiated).
   *
   * @param aIsVerticalSwipe
   *        Whether we're dealing with a vertical swipe or not.
   */
  startAnimation: function HSA_startAnimation(aIsVerticalSwipe) {
    this._direction = aIsVerticalSwipe ? "vertical" : "horizontal";

    if (this.isAnimationRunning()) {
      // If this is a horizontal scroll, or if this is a vertical scroll that
      // was started while a horizontal scroll was still running, handle it as
      // as a fast swipe. In the case of the latter scenario, this allows us to
      // start the vertical animation without first loading the final page, or
      // taking another snapshot. If vertical scrolls are initiated repeatedly
      // without prior horizontal scroll we skip this and restart the animation
      // from 0.
      if (this._direction == "horizontal" || this._lastSwipeDir != "") {
        gBrowser.stop();
        this._lastSwipeDir = "RELOAD"; // just ensure that != ""
        this._canGoBack = this.canGoBack();
        this._canGoForward = this.canGoForward();
        this._handleFastSwiping();
      }
    }
    else {
      this._startingIndex = gBrowser.webNavigation.sessionHistory.index; <-- Causes CPOW warning
      this._historyIndex = this._startingIndex;
      this._canGoBack = this.canGoBack();
      this._canGoForward = this.canGoForward();
      if (this.active) {
        this._addBoxes();
        this._takeSnapshot();
        this._installPrevAndNextSnapshots();
        this._lastSwipeDir = "";
      }
    }
    this.updateAnimation(0);
  },

and later in the same file:

  /**
   * Stops the swipe animation.
   */
  stopAnimation: function HSA_stopAnimation() {
    gHistorySwipeAnimation._removeBoxes();
    this._historyIndex = gBrowser.webNavigation.sessionHistory.index; <-- Causes CPOW warning
  },
tracking-e10s: ? → +

Comment 1

3 years ago
Currently, the swipe gesture works anywhere that is not scrollable, e.g. the toolbar, findbar, (outer) window, or the top of the sidebar (but not sidebar content or tabbar since they could scroll). However it *should* only work within the content. 

Reworking this to only work in content may fix this issue since swipe detection can happen in the content process and only the navigation event needs to be reported to the main thread. This may or may not help with Bug 927702, too.
Hardware: x86 → All
tracking-e10s: + → m8+
tracking-e10s: m8+ → ---
Whiteboard: [unsafe-cpow-usage]
I am not seeing this unsafe CPOW usage warning now, but I want to dig into _why_ this got fixed...
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(mconley)
Resolution: --- → WORKSFORME
Nope, I'm wrong. This is still around.
Status: RESOLVED → REOPENED
Flags: needinfo?(mconley)
Resolution: WORKSFORME → ---
Re-nomming - this is definitely still around.
tracking-e10s: --- → ?
Whiteboard: [unsafe-cpow-usage]
tracking-e10s: ? → m8+
Assignee: nobody → mconley
Assignee: mconley → gwright
Stealing because gw280 is on PTO.
Assignee: gwright → mconley
Hey Neil - as you're already in progress fixing bug 1148505, do you think you'd also like to take this bug as well?
Flags: needinfo?(enndeakin)
(Assignee)

Updated

2 years ago
Assignee: mconley → enndeakin
Flags: needinfo?(enndeakin)
(Assignee)

Comment 7

2 years ago
Created attachment 8680012 [details] [diff] [review]
patch?

This is a patch that just uses the session history instead. This won't be 100% accurate is some cases.

I can't get the gesture to work very reliably at all anyway, so it's hard to tell if this patch works or not. I've spent too much time on this already.
(Assignee)

Comment 8

2 years ago
Created attachment 8691463 [details]
Updated patch

This patch gets the updated history index from session store. It seems to work ok for me. The animation mode, disabled by default, doesn't work anyway (bug 1170032) so I didn't particularly test it.
Attachment #8680012 - Attachment is obsolete: true
Attachment #8691463 - Flags: review?(felipc)
Attachment #8691463 - Flags: review?(felipc) → review+
(Assignee)

Comment 9

2 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/696662fec7bf036aa62d259d08bcaceb055c4ac2
Bug 1133569, remove cpow usage from back/forward gesture, r=felipe

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/696662fec7bf
Status: REOPENED → RESOLVED
Last Resolved: 2 years ago2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
You need to log in before you can comment on or make changes to this bug.