Opened 4 years ago

Closed 4 years ago

Last modified 3 years ago

#19212 closed defect (fixed)

SIGSEGV on particular website with developer tools open

Reported by: cypherpunks Owned by: tbb-team
Priority: Very High Milestone:
Component: Applications/Tor Browser Version:
Severity: Critical Keywords: tbb-crash, noscript
Cc: arthuredelstein, brade, mcs Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

System: linux 64bit (arch linux), gnome3, Tor browser 6.0

Actions to reproduce:
1) Open F12 tools (Network tab if it matters)
2) Go to https://archive.org/details/a2_Zork_I_The_Great_Underground_Empire_1980_Infocom
3) Click on the play button.
4) After some downloads, the browser crashes.

I've tested this with both the security slider on low as on medium-high and that changes nothing.
I went ahead and downloaded the debug systems and got a backtrace in gdb, which I'll include.

If there is anything else that I can help with, do let me know!

Child Tickets

Attachments (2)

backtrace (8.8 KB) - added by cypherpunks 4 years ago.
backtrace from gdb
19212-MacOS-Debug-CrashLog.txt (17.7 KB) - added by mcs 4 years ago.
log from Mac OS debugger session

Download all attachments as: .zip

Change History (24)

Changed 4 years ago by cypherpunks

Attachment: backtrace added

backtrace from gdb

comment:1 Changed 4 years ago by bugzilla

Confirmed on Win 7:

Faulting application name: firefox.exe, version: 45.1.1.0, time stamp: 0x00000000
Faulting module name: xul.dll, version: 45.1.1.0, time stamp: 0x00000000
Exception code: 0xc0000005
Fault offset: 0x025f8a22

comment:2 Changed 4 years ago by gk

Cc: arthuredelstein brade added
Keywords: tbb-crash added
Priority: MediumVery High
Severity: NormalCritical

A first look at it: this is due to one of our patches and it is already available on 6.0a5. Arthur/Mark/Kathy have any of you time to start working on this. I can continue tomorrow if you run out of time today.

comment:3 Changed 4 years ago by gk

Cc: mcs added

comment:4 Changed 4 years ago by mcs

I can also reproduce this on Mac OS. I am out of time for today, but I will attach a debugger session log for a debug build, where an assertion fails inside the JS interpreter.

Changed 4 years ago by mcs

log from Mac OS debugger session

comment:5 Changed 4 years ago by mcs

Two additional things:

  • For me (on Mac OS), the crash only occurs when the developer tools are open and the Network tab is active (although the game does not load in other cases; I always see a "Failed to download game data" message).
  • The crash still occurs when I set dom.indexedDB.enabled = true.

comment:6 Changed 4 years ago by arthuredelstein

FWIW, I noticed that if I disable the NoScript extension (using the Add-Ons Manager) then the crash does not occur. On the other hand, if I use Firefox 45 ESR with NoScript installed, then I also do not see the crash.

comment:7 in reply to:  5 ; Changed 4 years ago by gk

Replying to mcs:

Two additional things:

  • For me (on Mac OS), the crash only occurs when the developer tools are open and the Network tab is active (although the game does not load in other cases; I always see a "Failed to download game data" message).

That might be a related but different issue. I am still bisecting but I can see the "Failed to download game data" with the selected Network tab on https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.1.1esr-6.0-1&id=ff819d20657c0692e7005e11d04adffab4dd5efb while this is not crashing Tor Browser yet (while later commits are doing that).

comment:8 Changed 4 years ago by gk

https://gitweb.torproject.org/tor-browser.git/commit/?h=tor-browser-45.1.1esr-6.0-1&id=0b8b418640999e20218833e1ba188246bdd280f1 seems to be the culprit. I'll double check with two clobber builds in order to rule bisecting failures out. But I guess it is quite safe to work from the assumption that this patch is causing this.

comment:9 Changed 4 years ago by gk

Yes, confirmed. This is the problematic patch. Now, what went wrong...

comment:10 in reply to:  7 Changed 4 years ago by gk

Replying to gk:

Replying to mcs:

Two additional things:

  • For me (on Mac OS), the crash only occurs when the developer tools are open and the Network tab is active (although the game does not load in other cases; I always see a "Failed to download game data" message).

That might be a related but different issue.

That's actually the case. This is due to us using private browsing mode and is reproducible in a vanilla Firefox as well.

comment:12 Changed 4 years ago by gk

Okay, my time is up for now and I did not track it to a good next intermediate step down :(

comment:13 in reply to:  11 Changed 4 years ago by mcs

Replying to gk:

So far: this is due to yield connectTimeline(); in https://mxr.mozilla.org/mozilla-esr45/source/devtools/client/netmonitor/netmonitor-controller.js#243

Good find. Avoiding connectTimeline() makes it do timeline events are not created, which avoids the codepath where Tor Browser crashes.

After trying for a while to find the root cause of this issue, Kathy and I started to look and see if Mozilla made any changes after ESR45 in code near the crash location. We found the following bug which looks like a duplicate of this ticket:

https://bugzilla.mozilla.org/show_bug.cgi?id=1258535

The crash does disappear when I disable NoScript. Mozilla made two fixes, one to add more assertions and one to add an additional null pointer check:

https://hg.mozilla.org/mozilla-central/rev/cd0123e0a09d
https://hg.mozilla.org/mozilla-central/rev/029c36687f2f

I am not convinced that they fixed the bug though. Applying their second patch did not fix this crash. Kathy and I are about out of time for today, but hopefully this gets us a little closer to a solution.

comment:14 Changed 4 years ago by mcs

On second thought, this may not be an exact duplicate of the Mozilla bug because of our dependency on the #16528 patch. But it sure is close.

comment:15 in reply to:  14 ; Changed 4 years ago by gk

Replying to mcs:

On second thought, this may not be an exact duplicate of the Mozilla bug because of our dependency on the #16528 patch. But it sure is close.

It is not. I applied both fixes but the issue remains. I still don't understand why cx->compartment() returns a null pointer with our patch applied. One way to handle that could be to do simply return false in this case, quick and dirty. I am not that happy about this solution though, as this introcuces a second order patch which might have issues on its own.

Another way forward would be trying to understand why our patch is causing issues. I am still trying to wrap my head around the fact that basically just falling back to the PBM permission check is causing this. Why does commenting out those two code blocks cause our problem given that a normal Firefox in PBM does not care about those two code blocks either but does not crash anymore. (after bug 125835.

A third approach might be to understand what role NoScript plays. Why is it causing cx->compartment() to be null. We could ask Giorgio if he'd merge a patch for us.

I think I prefer option 2) right but time is up for now. I am back again in a couple of hours and I still have hope we can get that fixed in 6.0.1.

comment:16 Changed 4 years ago by mcs

Here is another piece of data: Kathy and I can reproduce the crash after backing out the #16528 patch if we also flip dom.indexedDB.enabled to true (and stay in private browsing mode). In fact, we can also reproduce it with Firefox 46.0.1 in private browsing mode on Mac OS using a clean profile that has NoScript installed (with "Allow Scripts Globally" on).

We cannot reproduce it using Firefox Developer Edition (48.0a2 2016-06-02) using the same profile.

comment:17 Changed 4 years ago by mcs

It seems there are more of these kinds of problems lurking in the Firefox code. See: https://bugzilla.mozilla.org/show_bug.cgi?id=1248948#c0 (I can reproduce that crash in TB 6.0).

Mozilla's fixes look like "bandaids" to me; the comments in the Mozilla bugs we have found so far do not indicate a deep understanding of the problem(s).

comment:19 in reply to:  18 ; Changed 4 years ago by gk

Isn't that https://bugzilla.mozilla.org/show_bug.cgi?id=1258535 (which got fixed in mozilla48)? (re comment:17)

Last edited 4 years ago by gk (previous) (diff)

comment:20 in reply to:  19 Changed 4 years ago by mcs

Replying to gk:

Isn't that https://bugzilla.mozilla.org/show_bug.cgi?id=1258535 (which got fixed in mozilla48)? (re comment:17)

There is a different patch for it:
https://hg.mozilla.org/mozilla-central/rev/1e5447cbadfa

comment:21 Changed 4 years ago by mcs

Since Kathy and I are out of time for today, we created a workaround patch (in case that is the best thing anyone can come up with):
https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug19212-01&id=d7f98ce46b9cc43bb4ec8482938cb04f6ca99e21

comment:22 Changed 4 years ago by gk

Resolution: fixed
Status: newclosed

Arthur is fine with it, too, so taking for 6.0.1. In fact all other possible fixes would likely have been more complex making them more unsuitable for the stable. This is commit d7f98ce46b9cc43bb4ec8482938cb04f6ca99e21 on tor-browser-45.1.1esr-6.0-1.

comment:23 in reply to:  15 Changed 3 years ago by bugzilla

Keywords: noscript added

what role NoScript plays. Why is it causing cx->compartment() to be null. We could ask Giorgio if he'd merge a patch for us.

Note: See TracTickets for help on using tickets.