Segmentation fault with "high" security when changing in about:addons to "Extensions" or "Appearance"

Tor-Browser version: 7.0a3-build4, 64 bit Linux

The segmentation fault is maybe only the most annoying part of the underlying issue. If the security level is set to "High" and after a restart of Tor Browser, opening about:addons and trying to change to the "Extensions" or "Appearance" panel leads to a segmentation fault.

Other effects of setting the security level to "High" which may be related or may help you finding the root cause: (after closing and restarting)

• the symbols in front of the panel descritions in about:preferences are not visible
• the checkboxes in about:preferences don't show a check mark or a dot if they are selected

Unfortunately I don't know how to debug this, but I hope you can reproduce this easily.

Trac:
Username: viktorj

added TorBrowserTeam201704R in Legacy / Trac component::applications/tor browser in Legacy / Trac ff52-esr in Legacy / Trac owner::mcs in Legacy / Trac priority::very high in Legacy / Trac reporter::viktorj in Legacy / Trac resolution::fixed in Legacy / Trac severity::major in Legacy / Trac sponsor::4 in Legacy / Trac status::closed in Legacy / Trac tbb-7.0-must-alpha in Legacy / Trac tbb-crash in Legacy / Trac tbb-usability in Legacy / Trac type::defect in Legacy / Trac labels

Might be good to get that squeezed into the final 7.0a3 if possible.

Trac:
Cc: viktor_jaegerskuepper@freenet.de to viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade
Severity: Normal to Major
Keywords: N/A deleted, tbb-usability, TorBrowserTeam201704, tbb-7.0-must-alpha, ff52-esr, tbb-crash added
Priority: Medium to Very High
Sponsor: N/A to Sponsor4

Trac:
Owner: tbb-team to arthuredelstein
Status: new to accepted

Block SVG in content: now all pages are content (e10s?). What is with new ff patch, the same?

Replying to cypherpunks:

Block SVG in content: now all pages are content (e10s?). What is with new ff patch, the same?

Indeed this problem seems to be related to the svg.in-content.enabled pref. If I choose High Security but set this pref to "true" then the problems in the description (including the crash) go away.

Trac:
Cc: viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade to viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade, gk

FWIW: This is not e10s related. We are crashing somewhere in CSS code:

#0  nsAttrValue::Contains (this=this@entry=0xa5a5a5a5a5a5a5a5, 
    aValue=0x7fffc869cb80, aCaseSensitive=eCaseMatters)
    at /home/debian/build/tor-browser/dom/base/nsAttrValue.cpp:1163
#1  0x00007ffff3a0024b in SelectorMatches (aElement=0x7fffc407d600, 
    aSelector=0x7fffd708e460, aNodeMatchContext=..., aTreeMatchContext=..., 
    aSelectorFlags=aSelectorFlags@entry=SelectorMatchesFlags::NONE, 
    aDependence=0x0, this=<optimized out>)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:1723
#2  0x00007ffff3a00d21 in SelectorMatches (aElement=aElement@entry=0x7fffc407d600, 
    aSelector=aSelector@entry=0x7fffd708e460, aNodeMatchContext=..., 
    aTreeMatchContext=..., 
    aSelectorFlags=aSelectorFlags@entry=SelectorMatchesFlags::NONE, 
    aDependence=aDependence@entry=0x0, this=<optimized out>)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:1670
#3  0x00007ffff3a00d6c in nsCSSRuleProcessor::RestrictedSelectorMatches (
    aElement=aElement@entry=0x7fffc407d600, aSelector=0x7fffd708e460, 
    aTreeMatchContext=...)
    at /home/debian/build/tor-browser/layout/style/nsCSSRuleProcessor.cpp:2299
#4  0x00007ffff3ac3085 in mozilla::ElementRestyler::SelectorMatchesForRestyle (
    this=0x7fffffffae20, aElement=0x7fffc407d600)
    at /home/debian/build/tor-browser/layout/base/RestyleManager.cpp:2537

Kathy and I are also looking at this ticket. Arthur, please let us know if you are making progress so we are not duplicating work.

We can reproduce the crash, and we believe that the immediate cause is the static_cast that is in this code from Element.cpp:

const nsAttrValue*
nsIContent::DoGetClasses() const
{
  MOZ_ASSERT(HasFlag(NODE_MAY_HAVE_CLASS), "Unexpected call");
  MOZ_ASSERT(IsElement(), "Only elements can have classes");

  if (IsSVGElement()) {
    const nsAttrValue* animClass =
      static_cast<const nsSVGElement*>(this)->GetAnimatedClassName();
    if (animClass) {
      return animClass;
    }
  }

  return AsElement()->GetParsedAttr(nsGkAtoms::_class);
}

But the above code is not new. Our current working theory is that SVGs are being blocked in error early during creation of the about:addons document (and possibly in other cases) even though they should be allowed. If some time later SVGs are perceived as allowed, then Bad Things will occur such as doing a static_cast to the wrong kind of object.

In theory, and hopefully in practice, the Mozilla patch to block SVGs is better than our approach because it assigns an alternate namespace for SVGs at element creation time, which should avoid these kinds of static_cast bugs.

Kathy and I tracked down the root cause of the crash (which is also causing SVG images to not appear in about:preferences). Apparently, for some subresource documents, SVG elements are created before the document is attached to the parent window. This causes NS_SVGEnabledForChannel() to fail to perform its whitelist check for documents such as toolkit/mozapps/extensions/content/extensions.xml (because we end up with a NULL topDocURI), which in turn causes SVGs to be disabled at first and later allowed (because ultimately the subresource is part of about:addons, which is whitelisted).

I am not sure what changed between Firefox 45 and 52 to cause this problem, but adding a check against the system principal in this specific case seems to fix things. It is also worth noting that Mozilla's patch for https://bugzilla.mozilla.org/show_bug.cgi?id=1216893 uses IsSystemPrincipal() checks too.

We will post a patch soon.

Shouldn't you just backport the Mozilla's patches?

I've been working on the backport approach, which does get rid of the crash. Unfortunately there is a lot missing from Mozilla's version that needs to be added back from Kathy and Mark's old SVG patch and I'm still debugging some issues. I'm opening a new ticket (legacy/trac#22002 (moved)) for the backport, because I think we should go with Mark and Kathy's old patch (with fixup) for now.

Trac:
Status: accepted to new

Here is our patch: https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug21962-01&id=4cc114a57704a76988cb7862c6be2fd94639d144

Trac:
Status: new to needs_review
Keywords: TorBrowserTeam201704 deleted, TorBrowserTeam201704R added

legacy/trac#22008 (moved) is a duplicate.

Trac:
Cc: viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade, gk to viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade, gk, notanon740

legacy/trac#22023 (moved) is a duplicate.

Trac:
Cc: viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade, gk, notanon740 to viktor_jaegerskuepper@freenet.de, arthuredelstein, mcs, brade, gk, notanon740, Dbryrtfbcbhgf

Trac:
Status: needs_review to assigned
Owner: arthuredelstein to mcs

Trac:
Status: assigned to needs_review

This bug in content crashes the parent process that shouldn't be at any circumstances. e10s design flaw?

Replying to mcs:

Here is our patch: https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug21962-01&id=4cc114a57704a76988cb7862c6be2fd94639d144

Thanks for tracking this down and providing this fix. Good job! I've tested the fix on Windows and a 32bit Linux system and it fixes the issue. The code looks good to me as well.

Arthur, what do you think?

Replying to cypherpunks:

This bug in content crashes the parent process that shouldn't be at any circumstances. e10s design flaw?

Both content and chrome callers go through the same codepath for SVGs, so I would say the bug was not limited to content.

This patch also looks good to me. A couple of nitpicky questions occur to me:

• Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?
• I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

• Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

• I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

Trac:
Status: needs_review to needs_information

Replying to mcs:

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

• Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

• I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

I am with you here, let's leave this as-is. Arthur: anything else or do you think this is fine for the alpha?

Replying to gk:

Replying to mcs:

Replying to arthuredelstein:

This patch also looks good to me. A couple of nitpicky questions occur to me:

• Are there cases when checkedSystemPrincipal is false but topDocSpec remains empty?

Yes, for example if isSVGAllowed is set to true because the load context is not content.

Gotcha.

• I wonder if it might be worth moving the appropriate printf statements into the if...else part around line 150. Might simplify the code a bit.

Maybe, but Kathy and I prefer to leave the debug logging together near the end of the function to ensure that something is logged in all cases (and to make it easier to determine that is the case).

Georg, what do you think?

I am with you here, let's leave this as-is. Arthur: anything else or do you think this is fine for the alpha?

I think this patch is fine. Thanks for the clarifications.

This is commit 2d386f0b on tor-browser-52.1.0esr-7.0-2 now. Thanks all.

Trac:
Resolution: N/A to fixed
Status: needs_information to closed

closed

mentioned in issue legacy/trac#22002 (moved)

mentioned in issue legacy/trac#22008 (moved)

mentioned in issue legacy/trac#22023 (moved)

moved from legacy/trac#21962 (moved)

added Bug UX labels and removed 1 deleted label

removed 1 deleted label