793848 - Crash with <svg:use> removed during its own scrollbar attribute mutation event

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: testcase (crashes Firefox when loaded)

bp-0cada683-3e71-482d-8d98-0c2d92120924

I'm guessing this is a regression from bug 778810, which added the function mozilla::ScrollbarActivity::ActivityOccurred.

Why does scrolling set DOM attributes?

Comment 1

[Jesse Ruderman]

Attachment: stack traces: (1) mutation event listener, (2) crash

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Why do we end up dispatching mutation event. Scrollbars should be native anonymous.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Shouldn't rely on the element type, but frame type.
In the test we end up getting parent frame which is ScrollFrame.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

As far as I see, this is a really old bug.

Comment 5

[Scoobidiver (away)]

I loaded the testcase on Windows:
* 15.0.1: no crash
* 16.0b4: no crash
* 17.0a2: bp-101eb84b-d2b6-4f41-9dbf-37a5c2120926
* 18.0a1: bp-5894bccb-d4ac-448a-b32b-4c32d2120926

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Sure, using *that* testcase. But I don't see why the problem wouldn't happen with some other
testcase too.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 664662 [details] [diff] [review]
patch

r=me

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 664662 [details] [diff] [review]
patch

[Security approval request comment]
How easily can the security issue be deduced from the patch?
I'd say quite easily. Just try to make svg:use to use some unusual frame type

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
NA

Which older supported branches are affected by this flaw?
All

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should work in older branches too.
(if the patch doesn't apply cleanly, the changes should be minor
syntactic changes. The code is several years old.)

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe. In normal cases svg:use should behave like svg:use.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 664662 [details] [diff] [review]
patch

sec-approval+ assuming you can get branch landing approval from drivers

Comment 10

[Alex Keybl [:akeybl]]

(In reply to Olli Pettay [:smaug] from comment #8)
> Which older supported branches are affected by this flaw?
> All

Even though we don't crash in 15/16? Why are those branches marked as unaffected?

We're leaning towards not taking this for FF16 to limit risk, since this was internally reported.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

So I can certainly get access to supposed-to-be-native-anonymous element in FF16. So far no
crash, but I don't see why.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

If we don't take this to FF16, should we land the patch closer to FF17 release?

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 664662 [details] [diff] [review]
patch

Longstanding issue, internally reported, and FF16 doesn't crash. I don't think this qualifies for our final beta. Setting the sec-approval flag back to ? to answer the question of when to land on Central/Aurora/(next Beta).

Comment 14

[Daniel Veditz [:dveditz]]

We'll want a test for this that we can check in once we've release Fx17.

Comment 15

[Daniel Veditz [:dveditz]]

Comment on attachment 664662 [details] [diff] [review]
patch

sec-approval+, re-requesting branch approvals.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/mozilla-central/rev/ae051b7f7e4e

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e77259feea47
https://hg.mozilla.org/releases/mozilla-aurora/rev/dcc60baeca75
https://hg.mozilla.org/releases/mozilla-esr10/rev/92ef4d0a9c88

Comment 18

[Mats Palmgren (inactive)]

Crash test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb0da62e1348

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/eb0da62e1348

Comment 20

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old