989711 - Crash in nsContentIterator::NextNode with Range.toString

Status: RESOLVED WORKSFORME

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

Description

[Jesse Ruderman]

Attachment: testcase

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Jesse Ruderman]

bp-4c1a1449-d15f-4410-ba97-d5ccd2140330

Comment 3

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

So we end up having a text node and a NAC text node as range boundaries.
Content can't access that NAC, but Range's toString certainly can't handle such odd case.

Comment 4

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

But I don't quite understand what modify() does here.

Comment 5

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

Ah, we have this special Range which Find creates and that lets one to go through NAC boundaries.

Comment 6

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

I think we should just make sure that selection.getRangeAt never returns such ranges to JS which
cross NAC boundaries.

Comment 7

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

And we need to make sure such odd range objects don't affect to .modify() and such.

Comment 8

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

Attachment: ugly wip for the first case

Comment 9

[(no longer active)]

Comment on attachment 8399170 [details] [diff] [review]
ugly wip for the first case

This is truly terrible!

I think the real question is, why does document.getSelection() returns a selection object which lets you modify the find selection?  All kinds of things can go wrong with that, and I think that is the underlying problem.

Comment 10

[(no longer active)]

Hmm, actually, I guess if toString() is called on the range object from chrome then the same problem happens?

Comment 11

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

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #9)
> Comment on attachment 8399170 [details] [diff] [review]
> ugly wip for the first case
> 
> This is truly terrible!
:)

 
> I think the real question is, why does document.getSelection() returns a
> selection object which lets you modify the find selection?  All kinds of
> things can go wrong with that, and I think that is the underlying problem.
As long as we have window.find, I think it is rather useful to let it to change selection and
get the results from .getSelection()

But we must not return stuff which points to anonymous content to web.

Comment 12

[Jesse Ruderman]

(In reply to Olli Pettay [:smaug] from comment #6)
> I think we should just make sure that selection.getRangeAt never returns
> such ranges to JS which
> cross NAC boundaries.

In addition to Selection.getRangeAt, are similar changes needed for Selection.toString and Selection.deleteFromDocument?

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Olli, based on the history of comments here, do we understand the problem and possibly get a fix out there? I was reviewing the top crashes for last week on Aurora44 and this is #4 at the moment. Thanks!

Comment 14

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

Are you sure it is this, and not https://bugzilla.mozilla.org/show_bug.cgi?id=1224101 ?

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Olli Pettay [:smaug] from comment #14)
> Are you sure it is this, and not
> https://bugzilla.mozilla.org/show_bug.cgi?id=1224101 ?

Yes, you are correct! I noticed that in the last 3 days of data, this crash signature has been dropping so we may have fixed it as needed by backing out the patches in bug 1224101. Thanks and sorry for the mistake!

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'nsContentIterator::NextNode':
 - nightly (version 53): 0 crashes from 2016-11-14.
 - aurora  (version 52): 0 crashes from 2016-11-14.
 - beta    (version 51): 1 crash from 2016-11-14.
 - release (version 50): 9 crashes from 2016-11-01.
 - esr     (version 45): 8780 crashes from 2016-07-06.

Crash volume on the last weeks (Week N is from 01-02 to 01-08):
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       0       0       0       0       0
 - aurora        0       0       0       0       0       0       0
 - beta          0       1       0       0       0       0       0
 - release       1       3       1       1       2       0       1
 - esr         160     265     466     429     426     386     426

Affected platforms: Windows, Mac OS X, Linux

Crash rank on the last 7 days:
           Browser   Content   Plugin
 - nightly
 - aurora
 - beta
 - release
 - esr     #30

Comment 17

[Timea Cernea [:tbabos][inactive]]

Closing this as resolved:worksforme, could not reproduce the crash on MacOS using the attached testcase on the latest Nightly and there are no crashes in the last 6 months with this signature.