Closed Bug 358106 Opened 18 years ago Closed 18 years ago

Make ranges use nsIMutationObserver

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

References

(Depends on 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Sync to tip
(deleted), patch
Details | Diff | Splinter Review
Add couple of more assertions
(deleted), patch
jst
: review+
jst
: superreview+
Details | Diff | Splinter Review
We currently have special notifications just for ranges, along with a pile of code for nodes to keep track of which ranges are inside them. It'd be much more efficient and much less code to simply make ranges use nsIMutationObserver. Patch to make it so coming up.
Attached patch Patch to fix (obsolete) (deleted) — — Splinter Review
This'll take care of it. Most of this patch is just killing code no longer needed. The reason for passing around a pointer to a struct rather than 4 separate parameters in nsIMutationObserver::CharacterDataChanged is that I didn't want overhead of so many extra parameters that most callees don't care about anyway. The patch applies on top of the patch in bug 330872, but should be self explanatory as is. The only place where the patch doesn't apply as is is nsGenericDOMDataNode.cpp.
Attachment #243565 - Flags: superreview?(jst)
Attachment #243565 - Flags: review?(jst)
Comment on attachment 243565 [details] [diff] [review] Patch to fix Very nice! r+sr=jst
Attachment #243565 - Flags: superreview?(jst)
Attachment #243565 - Flags: superreview+
Attachment #243565 - Flags: review?(jst)
Attachment #243565 - Flags: review+
Attached patch Sync to tip (deleted) — — Splinter Review
Attachment #243565 - Attachment is obsolete: true
Checked in, and with no redness!
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Attached patch Add couple of more assertions (deleted) — — Splinter Review
Figured these could be good to have as well.
Attachment #244658 - Flags: superreview?(jst)
Attachment #244658 - Flags: review?(jst)
I see editor abusing the range API again. This time it's creating a range on a document during its destruction. The nodes still think they're in a document although the <HTML> has no parent node. This triggers ###!!! ASSERTION: Wrong root: '!aRoot || (nsContentUtils::ContentIsDescendantOf(aStartN, aRoot) && nsContentUtils::ContentIsDescendantOf(aEndN, aRoot))', file c:/trunk/mozilla/content/base/src/nsRange.cpp, line 405
Ah, so because the document is being destroyed, an input element wants to save its state. To do this it needs to get its value, which involves serializing a range obtained by selecting the contents of its native anonymous <div>...
What's aRoot in that case? The document? And in general, stack, steps to reproduce? Sounds like the usual "nsDocument::Destroy should die" thing.
Yes, aRoot is the document. Stack: nsRange::DoSetRange Line 405 nsRange::SelectNodeContents Line 731 nsPlaintextEditor::GetAndInitDocEncoder Line 1229 nsPlaintextEditor::OutputToString Line 1268 nsTextControlFrame::GetValue Line 2640 nsTextControlFrame::GetFormProperty Line 1999 nsHTMLInputElement::GetValue Line 674 nsHTMLInputElement::SaveState Line 2474 nsGenericHTMLFormElement::UnbindFromTree Line 3135 nsHTMLInputElement::UnbindFromTree Line 1811 nsGenericElement::UnbindFromTree Line 1873 (lots of times) nsHTMLBodyElement::UnbindFromTree Line 427 nsGenericElement::UnbindFromTree Line 1873 nsDocument::Destroy Line 4999 DocumentViewerImpl::Destroy Line 1573 nsSHistory::EvictWindowContentViewers Line 823 nsSHistory::EvictContentViewers Line 654 DocumentViewerImpl::Show Line 1925 nsPresContext::EnsureVisible Line 1358 PresShell::UnsuppressAndInvalidate Line 4856 PresShell::UnsuppressPainting Line 4905 DocumentViewerImpl::LoadComplete Line 1105 nsDocShell::EndPageLoad Line 4736 nsWebShell::EndPageLoad Line 975 nsDocShell::OnStateChange Line 4651 nsDocLoader::FireOnStateChange Line 1236 nsDocLoader::doStopDocumentLoad Line 869 nsDocLoader::DocLoaderIsEmpty Line 765 nsDocLoader::OnStopRequest Line 682 nsLoadGroup::RemoveRequest Line 685 nsDocument::DoUnblockOnload Line 5117 nsDocument::UnblockOnload Line 5065 nsImageLoadingContent::Event::~Event Line 803 nsImageLoadingContent::Event::`scalar deleting destructor'() nsRunnable::Release Line 51 nsCOMPtr<nsIRunnable>::~nsCOMPtr<nsIRunnable> Line 584 nsThread::ProcessNextEvent Line 492 NS_ProcessNextEvent_P Line 225 nsBaseAppShell::Run Line 153 + 0xc bytes nsAppStartup::Run Line 219 main1 Line 1245 main Line 1747 Steps to reproduce: well, generally any page navigation, but currently I have bfcache on and I suspect it might be more reliably reproducible with it off.
Actually, this is probably normal teardown where the parent elements have been unbound from the document, but the input element has not yet gotten UnbindFromTree called on it. So the <html> has no longer a parent, but the <input> still thinks its in the document. A solution might be to not use ranges to get the value.
In any case, we need a separate bug filed on it...
Especially if it happens to break form state restoration. Does it?
(In reply to comment #12) >Especially if it happens to break form state restoration. Does it? Yes, but for some reason, only for the focused field. Bug report coming up.
Depends on: 359674
Comment on attachment 244658 [details] [diff] [review] Add couple of more assertions r+sr=jst
Attachment #244658 - Flags: superreview?(jst)
Attachment #244658 - Flags: superreview+
Attachment #244658 - Flags: review?(jst)
Attachment #244658 - Flags: review+
Checked in last patch.
Depends on: 359919
Depends on: 415860
Component: DOM: Traversal-Range → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: