476975 - Crash running crashtests in [@ mozAutoDocUpdate ]

Status: RESOLVED DUPLICATE of bug 432114

(Core :: DOM: Editor, defect, P2)

Description

[Blake Kaplan (:mrbkap) (inactive)]

While running the crashtest for bug 428844, I occasionally crash setting up the contenteditable style sheet on the transformed document because the original document gets GCed after its window's mDocument has been updated. Because of this, by the time it calls ChangeContentEditableCount (which ends up calling nsHTMLDocument::TearingDownEditor, which clears out the style sheet's mDocument member).

One way to work around this would be to notify the original HTML document when it's about to be transformed by XSLT.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

...by the time it calls ChangeContentEditableCount from UnbindToTree, we're unable to tell it to tear down the editor (the patch in bug 428844 made this not crash on the null pointer, but left the dangling pointer since nobody tells that document that the editor's gone).

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

This should block -- at the very least it prevents me from running crashtests reliably and it is an exploitable crash.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

Here's one way to fix the bug. I'm not sure if this could possibly result in us removing the given stylesheets on a document that's already being edited? I suppose I could add a check somewhere that we don't do that. peterv, what do you think?

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Isn't this the bug you showed me where the fundamental problem is that editor is calling SetOwningDocument on a style sheet, but giving the style sheet a document that doesn't actually own the style sheet.  (Could we fix this by making the document own the style sheet?)

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #4)
> Isn't this the bug you showed me where the fundamental problem is that editor
> is calling SetOwningDocument on a style sheet,

Yes, it is.

> (Could we fix this by
> making the document own the style sheet?)

Well, the document doesn't really own the stylesheet, the editor does. It only temporarily grants ownership to the document (and then we reuse the sheet for multiple documents). I suppose we could fix that by not reusing the sheet, but to be honest, that's more work than I wanted to do for this bug. The patch just makes sure we restore ownership back to the editor before the document goes away.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is this related to bug 468353?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 361689 [details] [diff] [review]
patch v1

This doesn't actually fix the bug since by the time the destructor runs, we've already cleared the editing state off of the docshell.

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

It sounds like this bug may be fixed by my patch from bug 432114 comment 16. mrbkap, could you try that patch to see if it fixes your crash?

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

mrbkap, could you retest here? If it's fixed we should get it off the radar, if it's not we need to keep looking for a fix.

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

I don't have time to retest right now, but the patch in that bug definitely fixes the problem that I was seeing.

Comment 11

[Martijn Wargers (dead)]

Why does this bug need feedback? This was already marked as a duplicate. If this still needs feedback, then it shouldn't be marked as a duplicate, should it?