862309 - "ASSERTION: Trying to compute style without PresShell" with editor, cross-document selection

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Trying to compute style without PresShell: 'presShell', file /Users/jruderman/trees/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.cpp, line 592

###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file /Users/jruderman/trees/mozilla-central/layout/base/nsPresShell.cpp, line 6582

The "wrong shell" assertion was added in a security bug (bug 635852).

I'm guessing this will be fixed by not allowing selections to be in other documents.  That's part of the plan in bug 671152 and bug 768756, right?

Comment 1

[Jesse Ruderman]

Attachment: stacks (gdb)

Comment 2

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to Jesse Ruderman from comment #0)

> I'm guessing this will be fixed by not allowing selections to be in other
> documents.  That's part of the plan in bug 671152 and bug 768756, right?

Both unassigned :/

Comment 3

[(no longer active)]

Yeah we need somebody to finish them...

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Mats can you help figure out the way forward here?

Comment 5

[Mats Palmgren (inactive)]

Sorry, I don't have time to work on this.  BTW, Core/Editor security bugs
(and Range/Selection too) belongs to the DOM security group, not Layout.

Comment 6

[Andrew McCreight [:mccr8]]

Olli, how bad do you think this is?

Comment 7

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

This

Comment 8

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

This is bad. sg-crit

Comment 9

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to Jesse Ruderman from comment #0)> 
> ###!!! ASSERTION: Trying to compute style without PresShell: 'presShell',
> file
> /Users/jruderman/trees/mozilla-central/editor/libeditor/html/nsHTMLCSSUtils.
> cpp, line 592
In other words, this is bogus 

> 
> ###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file
> /Users/jruderman/trees/mozilla-central/layout/base/nsPresShell.cpp, line 6582
But this is bad

Comment 10

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: patch

Bug 671152 should be fixed, but that isn't something for branches.
So perhaps we should just add a simple if check for this case.
Mats, what do you think?

Comment 11

[Mats Palmgren (inactive)]

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

Yes, this is a good precaution.  It's still caused by bug(s) in our code
though so I'd like to keep the assertions in PresShell, so please add the
NS_ENSURE_STATE after the DEBUG block.  r=mats

Comment 12

[Mats Palmgren (inactive)]

Attachment: wip

Fwiw, I wonder if we could something like this to reject nodes not in
the same document as the Selection owner.  It would throw ERROR_FAILURE
which is probably wrong -- WRONG_DOCUMENT seems more appropriate.

BTW, is this the authoritative spec for selectAllChildren ?
https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections

Comment 13

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

Attachment: patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Not 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 really

Which older supported branches are affected by this flaw?
All, and looks like  Bug 635852 landed everywhere so this patch could also.
The bug itself is old, not a recent regression.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The patch should apply everywhere.

How likely is this patch to cause regressions; how much testing does it need?
Should be reasonable safe, but not risk free. Some testing is needed.

Comment 14

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

(In reply to Mats Palmgren [:mats] from comment #12) 
> Fwiw, I wonder if we could something like this to reject nodes not in
> the same document as the Selection owner.  It would throw ERROR_FAILURE
> which is probably wrong -- WRONG_DOCUMENT seems more appropriate.
I was thinking something similar but I think we just need bug 671152 fixed, and make sure the
spec is sane.


> BTW, is this the authoritative spec for selectAllChildren ?
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections
Kind of

Comment 15

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

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

sec-approval+ (for m-c) after 5/14. Please make and nominate patches for affected branches.

Comment 16

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

(In reply to Mats Palmgren [:mats] from comment #12)
> BTW, is this the authoritative spec for selectAllChildren ?
> https://dvcs.w3.org/hg/editing/raw-file/tip/editing.html#selections

It's the only spec I know of, but I sure wouldn't view it as authoritative.  More like, some research went into it and it's a good starting point for discussion.

Comment 17

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a8f45cb70707

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/ddef037b226b
https://hg.mozilla.org/releases/mozilla-esr17/rev/ef7a8f39085e

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ef0168b5cf1c
https://hg.mozilla.org/releases/mozilla-beta/rev/5a4e14b433bc

Comment 21

[Ioana (away)]

I tested this fix on Mac OSX 10.7 with Firefox 22 beta 2 debug and the testcase in this bug.

The first assertion in comment 0 has disappeared, but the second one is still there, just on another line:

###!!! ASSERTION: wrong shell: '!doc || doc->GetShell() == this', file ../../../layout/base/nsPresShell.cpp, line 6604.

Would you rather I filed a followup bug for that, or just reopen this one?

Comment 22

[Olli Pettay [:smaug][[e-mail]](vacation until July 14)]

That second one is there on purpose. See comment 11.
But we'll need to fix the code to not trigger that assertion.

Comment 23

[Jesse Ruderman]

Filed bug 876826.  Thanks for keeping that from slipping through the cracks, Ioana.

Comment 24

[Ioana (away)]

Verified as fixed on Firefox 17.0.7 ESR with the same results as in comment 21. Marking as verified...