830975 - [FIX] nsDocument::FlushPendingNotifications looks suspicious

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp?rev=fe51ccc53083#6715

Callers of FlushPendingNotifications should keep the document alive.
That doesn't always happen since, since even within 
nsDocument::FlushPendingNotifications itself it calls mParentDocument->FlushPendingNotifications,
and mParentDocument is a  member variable and doesn't guarantee that the object stays alive during the call
(if something ends up clearing the member variable).

Any objections if I add some nsCOMPtrs to keep stuff alive long enough?
Unfortunately that may show up in some profiles.

Comment 1

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

We do have crashes in nsDocument::FlushPendingNotifications

Comment 2

[Boris Zbarsky [:bzbarsky]]

Can we make the document hold a strong ref to itself only when it's really doing a flush?  I can probably live with the refcounting cost in that case...

Comment 3

[Daniel Veditz [:dveditz]]

several of the nsDocument::FlushPendingNotifications crashes (maybe a third?) are illegal writes, smells like a use after free.

Comment 4

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

Attachment: WIP

Maybe this way. Faster stack-only refcounting, which doesn't affect to
purple-ness of the object.
In normal cases this should add one
++, --, and a comparison to Flush. Should be quite fast.

Not super-pretty, but quite small and performance is critical here.

Comment 5

[Boris Zbarsky [:bzbarsky]]

It's worth testing this on the testcase in bug 709256 (which is close to a worst-case scenario for flush overhead).

Comment 6

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

ok. nsCOMPtr<nsIDocument> might actually not be so bad these days.
(and bug 789919 will make it faster)

Comment 7

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

Comment on attachment 703861 [details] [diff] [review]
WIP

Profiling this some more.

Comment 8

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

Attachment: a testcase

Comment 9

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

Comment on attachment 703861 [details] [diff] [review]
WIP

There is significant difference in performance (>10% opt linux64, non-pgo) when
comparing this approach to nsCOMPtr<nsIDocument>. The testcase is of course the worst case scenario.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 703861 [details] [diff] [review]
WIP

I'd still like to understand why we can't take a ref only in the cases when there's actually stuff to flush...

This is fine as far as it goes, but it seems like the other would be simpler.

Comment 11

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

(In reply to Boris Zbarsky (:bz) from comment #10)
> I'd still like to understand why we can't take a ref only in the cases when
> there's actually stuff to flush...
Ah, hmm, I guess I could use mNeedLayoutFlush/mNeedStyleFlush, and not really check whether something is flushed.

Comment 12

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

But contentsinks make this hard

Comment 13

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

And need to check if some ancestor flushes.

Comment 14

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

Comment on attachment 703861 [details] [diff] [review]
WIP

I can't now figure out anything simpler than this.

Comment 15

[Andrew McCreight [:mccr8]]

Comment on attachment 703861 [details] [diff] [review]
WIP

Review of attachment 703861 [details] [diff] [review]:
-----------------------------------------------------------------

Looks okay to me, though I don't know anything about FlushPendingNotifications in particular.

Comment 16

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

Comment on attachment 703861 [details] [diff] [review]
WIP

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think quite easily

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No, but patch itself is quite obvious.

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?
Patch seems to apply to m-c, aurora and beta, and needs just minor merging for esr17

How likely is this patch to cause regressions; how much testing does it need?
Should be relatively safe. Currently we may get crashes in the cases this patch fixes.

Comment 17

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

Boris, if you can think of a simpler fix, don't hesitate to tell it to me :)

Comment 18

[Al Billings [:abillings - ex-MoCo]]

I need release management to weigh in on when we want to take this since it affects everything and is obvious.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> But contentsinks make this hard
> And need to check if some ancestor flushes.

Ugh.  :(  Yeah, then what's here seems simplest.  :(

Comment 20

[Alex Keybl [:akeybl]]

(In reply to Al Billings [:abillings] from comment #18)
> I need release management to weigh in on when we want to take this since it
> affects everything and is obvious.

We're coming up on Beta 4 of FF19. Let's land on all branches this week. Is B2G affected Olli?

Comment 21

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

Everything is affected.

Comment 22

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

Comment on attachment 703861 [details] [diff] [review]
WIP

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Old stuff
User impact if declined: Possible sg:crit crashes
Testing completed (on m-c, etc.): Hasn't landed anywhere yet
Risk to taking this patch (and alternatives if risky): 
Shouldn't be risky. Alternatives are probably significantly slower and 
this is hot code.
String or UUID changes made by this patch: NA

Comment 23

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

https://hg.mozilla.org/mozilla-central/rev/0a453067b5de

Comment 24

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 703861 [details] [diff] [review]
WIP

Approving for all branches.

Comment 25

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

Landing real soon.

Comment 26

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

https://hg.mozilla.org/releases/mozilla-esr17/rev/fd2ee0d7d4a0
https://hg.mozilla.org/releases/mozilla-beta/rev/a75708877822
https://hg.mozilla.org/releases/mozilla-b2g18/rev/0e76f133a710
https://hg.mozilla.org/releases/mozilla-aurora/rev/932c91fda1cf

Comment 27

[Matt Wobensmith [:mwobensmith][:matt:]]

Is the attached test case applicable, and will it be set to review at some point?

Comment 28

[Ioana (away)]

I ran the test case attached to this bug, but I get the exact same behavior on Firefox 18 and Firefox 19 (beta 5). The counter on the page stays between 198 and 220 ms, and Firefox eats up about 50% of the CPU.

What should I be looking at to verify this fix?

Comment 29

[Boris Zbarsky [:bzbarsky]]

Nothing.  This is a proactive security fix for an issue we don't obviously have a way to trigger.  The testcase is just making sure the fix doesn't regress performance.