Opened 9 months ago

Closed 7 months ago

Last modified 7 months ago

#21267 closed defect (fixed)

e10s compatibility for Torbutton's content sizer

Reported by: mcs Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: tbb-torbutton, ff52-esr, tbb-7.0-must, TorBrowserTeam201703R
Cc: brade, arthuredelstein Actual Points:
Parent ID: #21201 Points:
Reviewer: Sponsor: Sponsor4

Description

Torbutton will need to be revised in various ways for compatiblity with Firefox's multiprocess mode (aka electrolysis or e10s). One example is the content sizer code. Here is the first error that Kathy and I saw:

A coding exception was thrown and uncaught in a Task.

Full message: TypeError: gBrowser.contentWindow is null
Full stack: quantizeBrowserSize/updateDimensions@chrome://torbutton/content/content-sizer.js:247:18
quantizeBrowserSize/autoresize/<@chrome://torbutton/content/content-sizer.js:436:7
TaskImpl_run@resource://gre/modules/Task.jsm:319:42
TaskImpl@resource://gre/modules/Task.jsm:277:3
createAsyncFunction/asyncFunction@resource://gre/modules/Task.jsm:252:14
Task_spawn@resource://gre/modules/Task.jsm:166:12
quantizeBrowserSize/autoresize@chrome://torbutton/content/content-sizer.js:431:3
quantizeBrowserSize/quantizeBrowserSizeMain/activate@chrome://torbutton/content/content-sizer.js:487:30
bindPref/update@resource://torbutton/modules/utils.js:31:24
bindPref@resource://torbutton/modules/utils.js:39:5
bindPrefAndInit@resource://torbutton/modules/utils.js:49:5
quantizeBrowserSize/quantizeBrowserSizeMain@chrome://torbutton/content/content-sizer.js:501:16
quantizeBrowserSize/stopObserving<@chrome://torbutton/content/content-sizer.js:508:5
observe/observer.observe@resource://torbutton/modules/utils.js:61:9

Child Tickets

Change History (15)

comment:1 Changed 9 months ago by cypherpunks

Keywords: tbb-torbutton added; torbutton removed
Status: newneeds_information

Are you going to enable that long-standing untested unaudited still buggy e10s in ff52-esr?!
Don't you think it requires separate task "switch to multi-process mode" after audit?
(Isn't content sizer code moving to Firefox?)

comment:2 in reply to:  1 ; Changed 9 months ago by mcs

Replying to cypherpunks:

Are you going to enable that long-standing untested unaudited still buggy e10s in ff52-esr?!
Don't you think it requires separate task "switch to multi-process mode" after audit?

Maybe. In any case, we want to prepare for the possibility of enabling multi-process mode, which means making sure our code is compatible with it.

(Isn't content sizer code moving to Firefox?)

I am not sure. Arthur?

comment:3 Changed 9 months ago by gk

Keywords: TorBrowserTeam201702 added

comment:4 Changed 9 months ago by gk

Sponsor: Sponsor4

Sponsor4 as well

comment:5 Changed 9 months ago by arthuredelstein

Parent ID: #21201

comment:6 in reply to:  2 Changed 9 months ago by arthuredelstein

Replying to mcs:

Replying to cypherpunks:

Are you going to enable that long-standing untested unaudited still buggy e10s in ff52-esr?!
Don't you think it requires separate task "switch to multi-process mode" after audit?

Maybe. In any case, we want to prepare for the possibility of enabling multi-process mode, which means making sure our code is compatible with it.

(Isn't content sizer code moving to Firefox?)

I am not sure. Arthur?

Sorry I missed this question. I think we do eventually want to move it to Firefox, but that's kind of a big task, so it maybe makes sense to fix it in torbutton for now, if it's not too hard.

comment:7 Changed 9 months ago by mcs

Cc: arthuredelstein added

Kathy and I spent some time looking at what it would take to make the content-sizer.js code work in an e10s environment. Unfortunately, it does not look like it will be a simple task and the existing code is complicated enough that it makes sense for Arthur to own this task.

Most of the problems are inside the updateDimensions() function. All places where the code accesses gBrowser.contentWindow must be modified to get information using an alternative method, or we need to use a content script combined with Firefox's message manager from the chrome side to request the info we need (full screen state, zoom factor, content window dimensions). The necessary changes will be a little tricky because simple variable access must be replaced with asynchronous message exchanges. Kathy and I do not understand the existing content-sizer code well enough to be confident that we will account for everything correctly when we rearchitect the code in this way.

comment:8 Changed 9 months ago by gk

Before we start spending a ton of work on a feature that is currently only in our alpha series (and there are no immediate plans to get it into stable shape): what about just backing it out for now? There have issues piled up in #14429 which we have not fixed which means we ship code in the alpha that is not even close to be stable code and it makes it harder branching off maintenance branches for the stable series: we need to always back out the feature or flip at least the preference accordingly. And: we want to have a fix for the underlying problem behind a pref in Firefox directly anyway.

comment:9 Changed 8 months ago by gk

Keywords: tbb-7.0-must added

comment:10 Changed 8 months ago by gk

Keywords: TorBrowserTeam201703 added; TorBrowserTeam201702 removed

Moving tickets to March

comment:11 in reply to:  8 Changed 8 months ago by arthuredelstein

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201703 removed
Status: needs_informationneeds_review

Replying to gk:

Before we start spending a ton of work on a feature that is currently only in our alpha series (and there are no immediate plans to get it into stable shape): what about just backing it out for now? There have issues piled up in #14429 which we have not fixed which means we ship code in the alpha that is not even close to be stable code and it makes it harder branching off maintenance branches for the stable series: we need to always back out the feature or flip at least the preference accordingly. And: we want to have a fix for the underlying problem behind a pref in Firefox directly anyway.

Yes, I think it's best to back it out for now. Here's a new ticket for implementing it in tor-browser.git: #21714.

And here's my patch for review:
https://github.com/arthuredelstein/torbutton/commit/21276

comment:12 Changed 7 months ago by mcs

r=brade, r=mcs
The backout patch looks good to us.

comment:13 in reply to:  12 Changed 7 months ago by arthuredelstein

Replying to mcs:

r=brade, r=mcs
The backout patch looks good to us.

Thanks for the review. I added the patch to my #21201 patch.

comment:14 Changed 7 months ago by arthuredelstein

Resolution: fixed
Status: needs_reviewclosed

comment:15 Changed 7 months ago by gk

We need to reset extensions.torbutton.maximize_warnings_remaining to 3 as well. I pushed a fixup for that: eadfd9622fc7a15aad5200e11a35a4f558ae5743.

Note: See TracTickets for help on using tickets.