Closed
Bug 1165982
Opened 9 years ago
Closed 9 years ago
Infinite loop in nsDOMIterator::AppendList
Categories
(Core :: DOM: Editor, defect)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox38 | --- | unaffected |
firefox38.0.5 | --- | unaffected |
firefox39 | --- | unaffected |
firefox40 | + | fixed |
firefox41 | + | fixed |
People
(Reporter: khuey, Assigned: froydnj)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
7.11 KB,
patch
|
ehsan.akhgari
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
874 bytes,
patch
|
ehsan.akhgari
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
STR: Visit http://www.philly.com/philly/news/20150518_Bucks_company_puts_pedal_to_metal_for_Amtrak.html#oJtB4v9JJ3SJ6GZ Click on the "Read N comments" link at the bottom. This triggers an infinite loop in an opt build and should trigger a fatal assertion in a debug build.
Updated•9 years ago
|
status-firefox38:
--- → unaffected
status-firefox38.0.5:
--- → unaffected
status-firefox39:
--- → ?
status-firefox40:
--- → affected
status-firefox41:
--- → affected
tracking-firefox39:
--- → ?
tracking-firefox40:
--- → ?
tracking-firefox41:
--- → ?
Comment 1•9 years ago
|
||
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8c56a1c8e458&tochange=afef0f347312 Suspect : Bug 1149163
Assignee | ||
Comment 2•9 years ago
|
||
OK, so we're failing because this call: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsEditorUtils.cpp#73 is failing. We're being called from here: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#7294 It looks like mDocChangeRange is empty at that point: (gdb) p *mDocChangeRange $3 = (nsRange &) @0x7fc282dfb320: {<nsIDOMRange> = {<nsISupports> = { _vptr.nsISupports = 0x7fc2ac37c290 <vtable for nsRange+16>}, <No data fields>}, <nsStubMutationObserver> = {<nsIMutationObserver> = {<nsISupports> = { _vptr.nsISupports = 0x7fc2ac37c408 <vtable for nsRange+392>}, <No data fields>}, <No data fields>}, <nsWrapperCache> = {_vptr.nsWrapperCache = 0x7fc2ac37c480 <vtable for nsRange+512>, mWrapper = {<js::HeapBase<JSObject*>> = {<No data fields>}, ptr = NULL}, mFlags = Python Exception <class 'gdb.error'> There is no member or method named mTagged.: 0}, mRefCnt = {mRefCntAndFlags = 5}, _mOwningThread = {mThread = 0x7fc29ea5e140}, static _cycleCollectorGlobal = {<nsXPCOMCycleCollectionParticipant> = {<nsScriptObjectTracer> = {<nsCycleCollectionParticipant> = { _vptr.nsCycleCollectionParticipant = 0x7fc2ac37c4d0 <vtable for nsRange::cycleCollection+16>, mMightSkip = false}, <No data fields>}, <No data fields>}, <No data fields>}, mOwner = [(nsHTMLDocument*) 0x7fc28de40000], mRoot = [(nsINode*) 0x0], mStartParent = [(nsINode*) 0x0], mEndParent = [(nsINode*) 0x0], mStartOffset = 0, mEndOffset = 0, mIsPositioned = false, mIsDetached = false, mMaySpanAnonymousSubtrees = false, mInSelection = false, mIsGenerated = false, mStartOffsetWasIncremented = false, mEndOffsetWasIncremented = false, mEnableGravitationOnElementRemoval = true, mAssertNextInsertOrAppendIndex = -1, mAssertNextInsertOrAppendNode = 0x0} and nsContentIterator doesn't like being Init'd with empty ranges like that. I guess the right thing to do here is to back out the part of bug 1149613 that made init'ing nsDOMIterator with ranges infallible, and re-insert failure checks wherever we need to? (For the moment, we don't need to back out the AppendList changes...)
Flags: needinfo?(nfroyd) → needinfo?(ehsan)
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1149613 landed on 40, so 39 is not affected.
tracking-firefox39:
? → ---
I was also able to repro it on firefix40 05-18 build. This is a high impact bug. Adding tracking flags for firefox40 and firefox41.
Assignee | ||
Comment 5•9 years ago
|
||
This simple patch fixes the page for me: I can now click on the "17 comments" link and have the comments show up at the bottom of the page. I didn't see any way to ask an nsRange "are you empty?", so having nsContentIterator do this checking seems like the next best thing... (What's not obvious to me is why the MOZ_ASSERT(aRange.GetStartParent()) wasn't firing, even though the range didn't *have* a StartParent...)
Attachment #8607530 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•9 years ago
|
||
Ehsan can weigh in on comment 2 in his comments on the patch.
Flags: needinfo?(ehsan)
Comment 7•9 years ago
|
||
Comment on attachment 8607530 [details] [diff] [review] provide fallible initialization of nsDOMIterator from an nsRange Review of attachment 8607530 [details] [diff] [review]: ----------------------------------------------------------------- Hmm, it seems to me like nsDOMSubtreeIterator is affected by this exact same issue, is it not? Also, can we get a test for this, please? Thanks!
Attachment #8607530 -
Flags: review?(ehsan) → review-
Assignee | ||
Comment 8•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #7) > Hmm, it seems to me like nsDOMSubtreeIterator is affected by this exact same > issue, is it not? Indeed. > Also, can we get a test for this, please? Thanks! Do you have suggestions on reducing test cases from large html/js pages?
Flags: needinfo?(ehsan)
Assignee | ||
Comment 10•9 years ago
|
||
OK, so my DOM-fu is not great enough to figure out how to write a proper testcase Part of the problem is that here: http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#277 we aren't checking that the SelectNode call actually succeeded. In the test page, we're winding up with |node| in that method being a <body> element; its NodePrincipal is a disqus page. But at this check: http://mxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#1331 the caller has a principal of the outer philly.com page. So the caller can't touch the <body> element, and the call fails. But we don't propagate that error outwards, so we assert later on. Breaking at that callsite in nsHTMLEditRules.cpp and doing things like DumpJSStack() give me inscrutable jQuery stacks. If we fix the propagation of that error, everything works. What I don't see is how to write a testcase. I have a skeleton testcase setup with what I think mimics the page setup: an outer page on one domain containing an iframe referencing a page on another domain. That gives me the principal mismatch that's causing problems. But I can't dynamically create the contenteditable div inside the iframe from the outer page. And if I try to have the inner page create it, the check from nsRange says that our "current" principal (i.e. nsContentUtils::SubjectPrincipal()) is the null principal. Which succeeds just fine. How can one create a contenteditable div with an "outer" principal differing from the principal of the page holding on to the div?
Flags: needinfo?(ehsan)
Comment 11•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #10) > OK, so my DOM-fu is not great enough to figure out how to write a proper > testcase > Part of the problem is that here: > > http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/ > nsHTMLEditRules.cpp#277 > > we aren't checking that the SelectNode call actually succeeded. In the test > page, we're winding up with |node| in that method being a <body> element; > its NodePrincipal is a disqus page. But at this check: > > http://mxr.mozilla.org/mozilla-central/source/dom/base/nsRange.cpp#1331 > > the caller has a principal of the outer philly.com page. So the caller > can't touch the <body> element, and the call fails. But we don't propagate > that error outwards, so we assert later on. FWIW this is all so stupid. We've been moving the editor away from using the Web facing DOM APIs, so that these security checks don't get in our way... :( > Breaking at that callsite in nsHTMLEditRules.cpp and doing things like > DumpJSStack() give me inscrutable jQuery stacks. > > If we fix the propagation of that error, everything works. It would be great if you fixed that too. :-) But I'd like both fixes, since this is one bug triggering another one... > What I don't see is how to write a testcase. I have a skeleton testcase > setup with what I think mimics the page setup: an outer page on one domain > containing an iframe referencing a page on another domain. That gives me > the principal mismatch that's causing problems. > > But I can't dynamically create the contenteditable div inside the iframe > from the outer page. And if I try to have the inner page create it, the > check from nsRange says that our "current" principal (i.e. > nsContentUtils::SubjectPrincipal()) is the null principal. Which succeeds > just fine. If all you need is for the outer page to tell the inner "hey, create me a contenteditable", how about postMessaging to it? But I suspect that won't trigger the bug, since the postMessage is async. Another thing you may try is to resize the iframe from the outer page, and in the inner, handle the resize event, add a contenteditable, and flush. But you shouldn't be getting a null principal anyway. Can I see your test case please? (Note that while it would be nice to have a test case here, it won't be the first editor bug for which I have been unable to add a minimized test case... :/ ) (Another thing which we can try is to ask wizards such as Martijn or Jesse to see if they know of better ways of writing minimized testcases...)
Flags: needinfo?(ehsan)
Comment 12•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #8) > Do you have suggestions on reducing test cases from large html/js pages? The brainless way to do it is by bisection. Save the file locally as a complete HTML page, then cut out chunks of the file to see if the error persists. Keep doing this until you have an HTML file where removing any line will make the error disappear. Repeat for each included file. This is not necessarily practical to do by hand for large pages. But I'm almost sure Jesse Ruderman has some sort of tool to do it automatically, based on some of the testcases he's submitted in the past -- although I don't know if it works for arbitrary pages. This also won't work at all if the bug depends on making network calls or things like that, obviously.
Assignee | ||
Comment 13•9 years ago
|
||
Here's the testcase that I'm working with. I double-checked things in the debugger this morning, and I'm actually getting the *system* principal, not the null principal as previously stated...but that seems pretty weird, too.
Attachment #8608100 -
Flags: feedback?(ehsan)
Comment 14•9 years ago
|
||
Comment on attachment 8608100 [details] [diff] [review] testcase for editor Review of attachment 8608100 [details] [diff] [review]: ----------------------------------------------------------------- On a second look, I'm 100% sure that I understand what's wrong now. When the contenteditable gets added, there is no JS stack frame on the stack, which means the system principal will be the subject principal. You need to use the resize trick, or some such. ::: editor/libeditor/tests/test_bug1165982.html @@ +17,5 @@ > +<script type="application/javascript"> > + > + /** Test for Bug 1165982 **/ > +SimpleTest.waitForExplicitFinish(); > +SimpleTest.waitForFocus(function() { Not quite sure why you're waiting for focus here...
Attachment #8608100 -
Flags: feedback?(ehsan) → feedback-
Comment 15•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11) > (Another thing which we can try is to ask wizards such as Martijn or Jesse > to see if they know of better ways of writing minimized testcases...) I guess Nathan already added testcase to the bug? In any case, I wasn't able to reproduce this on the http://www.philly.com site, using a 2015-05-20 trunk build on MacOSX10.9.5.
Assignee | ||
Comment 16•9 years ago
|
||
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11) > If all you need is for the outer page to tell the inner "hey, create me a > contenteditable", how about postMessaging to it? But I suspect that won't > trigger the bug, since the postMessage is async. Another thing you may try > is to resize the iframe from the outer page, and in the inner, handle the > resize event, add a contenteditable, and flush. I can trigger a resize, but it looks like the resize is handled async, not sync. Is there a trick to forcing sync resizes that I'm not aware of? My current code looks like: info("height before: " + iframe.getBoundingClientRect().height); // Force resize and emit debug information. iframe.width = 500; iframe.height = 1000; info("width: " + iframe.getBoundingClientRect().width); info("height: " + iframe.getBoundingClientRect().height); info("offsetTop: " + iframe.offsetTop); // This test fails for reasons that I am ignorant of. is(iframe.getBoundingClientRect().width, 500, "width of iframe should be 500 now"); SimpleTest.executeSoon(SimpleTest.finish); // dump() statements from my resize handler occur after this message, not before. info("finishing iframe load event handler");
Flags: needinfo?(ehsan)
Comment 17•9 years ago
|
||
We tried a while yesterday on IRC to get this test to work, and we pretty much gave up. :/
Flags: needinfo?(ehsan)
Assignee | ||
Comment 18•9 years ago
|
||
Now with nsDOMSubtreeIterator changes.
Attachment #8607530 -
Attachment is obsolete: true
Attachment #8609361 -
Flags: review?(ehsan)
Assignee | ||
Comment 19•9 years ago
|
||
If somebody has bright ideas on how to easily introduce script blockers, I think a testcase could be constructed based on what we already have.
Attachment #8609362 -
Flags: review?(ehsan)
Updated•9 years ago
|
Attachment #8609361 -
Flags: review?(ehsan) → review+
Comment 20•9 years ago
|
||
Comment on attachment 8609362 [details] [diff] [review] part 2 - validate an nsRange::SelectNode call in nsHTMLEditRules::Init Review of attachment 8609362 [details] [diff] [review]: ----------------------------------------------------------------- Ideally, we would add an internal version of nsRange::SelectNode that wouldn't do the security check and use it here, but I guess I can live with this for now...
Attachment #8609362 -
Flags: review?(ehsan) → review+
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/314e2ed158f7 https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb61ae99404
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/314e2ed158f7 https://hg.mozilla.org/mozilla-central/rev/7fb61ae99404
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8609361 [details] [diff] [review] part 1 - provide fallible initialization of nsDOMIterator from an nsRange This approval request is for part 1 and part 2 in this bug. Approval Request Comment [Feature/regressing bug #]: bug 1149163 [User impact if declined]: Some sites will not work correctly, particularly Disqus commenting. [Describe test coverage new/current, TreeHerder]: No automated test coverage, but manual verification that the affected site now works. [Risks and why]: I'd say low risk. This patch is essentially re-adding some error checking removed in bug 1149163. [String/UUID change made/needed]: None.
Attachment #8609361 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•9 years ago
|
Attachment #8608100 -
Attachment is obsolete: true
Updated•9 years ago
|
Attachment #8609361 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 24•9 years ago
|
||
Comment on attachment 8609362 [details] [diff] [review] part 2 - validate an nsRange::SelectNode call in nsHTMLEditRules::Init [Triage Comment]
Attachment #8609362 -
Flags: approval-mozilla-aurora+
Updated•9 years ago
|
Assignee: nobody → nfroyd
Comment 25•9 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/c49d7de6e59a https://hg.mozilla.org/releases/mozilla-aurora/rev/670f70e5143e
Updated•9 years ago
|
Keywords: regressionwindow-wanted
You need to log in
before you can comment on or make changes to this bug.
Description
•