Opened 20 months ago

Closed 19 months ago

Last modified 7 months ago

#21308 closed defect (fixed)

Fix modernizr breakage for TBB/ESR52

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

Description

Our patch for #16528 doesn't work with electrolysis enabled, so we need a new patch for TBB/ESR52.

Child Tickets

Change History (14)

comment:1 Changed 20 months ago by gk

Keywords: ff52esr added; tbb-52esr removed

comment:2 Changed 20 months ago by gk

Keywords: ff52-esr added; ff52esr removed

comment:3 Changed 20 months ago by gk

Keywords: TorBrowserTeam201702 added

comment:4 Changed 20 months ago by gk

Sponsor: Sponsor4

This is Sponsor4 work

comment:5 Changed 20 months ago by arthuredelstein

Keywords: TorBrowserTeam201702R added; TorBrowserTeam201702 removed
Status: newneeds_review

Here's a patch for review:
https://github.com/arthuredelstein/tor-browser/tree/21308

To explain this patch a little:

In private browsing mode, calling

var req = window.indexedDB.open("anydbname")

returns without throwing an error, but results in

req.name = "InvalidStateError".

Whereas dom.indexeddb.enabled = false, var req = window.indexedDB.open("anydbname") throws an InvalidStateError instead.

The Modernizr script fails to handle the latter case. So this patch changes the behavior of the code when dom.indexedb.enabled = false so that no error is thrown, but instead window.indexedDB = null. This is correctly handled by Modernizr, and also matches the convention where the use of a JS API is preceding by checking for its presence, e.g.:

if (window.indexedDB) {
  var req = indexedDB.open(...)
  // More indexedDB usage
}

I also tested this with Twitter and it appears to get it working properly again.

comment:6 Changed 20 months ago by mcs

r=brade, r=mcs
This looks good to us. Nice work!
We tested with the Modernizr script (using our own little test page) and the right thing seemed to happen.

comment:7 in reply to:  6 Changed 20 months ago by arthuredelstein

Replying to mcs:

r=brade, r=mcs
This looks good to us. Nice work!
We tested with the Modernizr script (using our own little test page) and the right thing seemed to happen.

Thanks for reviewing this! I added a small mochitest to make sure it's working correctly.
https://github.com/arthuredelstein/tor-browser/commit/21308+2
I will this add patch to the latest #20680 branch.

comment:8 in reply to:  description ; Changed 19 months ago by gk

Status: needs_reviewneeds_information

Replying to arthuredelstein:

Our patch for #16528 doesn't work with electrolysis enabled, so we need a new patch for TBB/ESR52.

Hm. Do you happen to know why that's the case? Because what we do with the patch we currently have is just disabling the pref checks and relying solely on private browsing mode (PBM) checks. And surely, IndexedDB is still disabled in PBM. Thus, those checks should still work as-is.

Second point, I assume the patch in comment:5 works as well with e10s disabled? Because it is still unclear right now how stable e10s will be in a Tor Browser ESR52. We might need to disable it or enable it later.

comment:9 Changed 19 months ago by gk

Keywords: TorBrowserTeam201703R added; TorBrowserTeam201702R removed

No February anymore.

comment:10 Changed 19 months ago by gk

Keywords: tbb-7.0-must added

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

Status: needs_informationneeds_review

Replying to gk:

Replying to arthuredelstein:

Our patch for #16528 doesn't work with electrolysis enabled, so we need a new patch for TBB/ESR52.

Hm. Do you happen to know why that's the case? Because what we do with the patch we currently have is just disabling the pref checks and relying solely on private browsing mode (PBM) checks. And surely, IndexedDB is still disabled in PBM. Thus, those checks should still work as-is.

The old patch was causing crashes with e10s. I don't know why.

Second point, I assume the patch in comment:5 works as well with e10s disabled? Because it is still unclear right now how stable e10s will be in a Tor Browser ESR52. We might need to disable it or enable it later.

In my hands it seems to be working OK with e10s enabled or disabled.

comment:12 Changed 19 months ago by gk

Okay, could you pick that on top of your up-to-date 20680 branch and close this ticket then?

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

Resolution: fixed
Status: needs_reviewclosed

Replying to gk:

Okay, could you pick that on top of your up-to-date 20680 branch and close this ticket then?

Done. The corresponding Mozilla bug is at https://bugzilla.mozilla.org/1192643

comment:14 Changed 7 months ago by arthuredelstein

Keywords: tbb-no-uplift added
Parent ID: #20680

For the record: in #23745 we reverted this patch because it was no longer needed. So we also won't need to uplift it.

Parent was: #20680

Note: See TracTickets for help on using tickets.