Feel free to grab this.

Comment on attachment 697160 [details] [diff] [review] add IsPointInRange() webidl version >+nsRange::IsPointInRange(nsINode& aParent, uint32_t aOffset, ErrorResult& rv) aRv

Comment on attachment 697156 [details] [diff] [review] add ComparePoint() webidl version >+nsRange::ComparePoint(nsINode& aParent, uint32_t aOffset, ErrorResult& rv) aRv

Comment on attachment 697168 [details] [diff] [review] add IntersectsNode() webidl version > >- NS_ENSURE_TRUE(mIsPositioned, NS_ERROR_NOT_INITIALIZED); >+bool >+nsRange::IntersectsNode(nsINode& aNode) >+{ >+ if (!mIsPositioned) { >+ return false; >+ } So you actually change the behavior here. Please add a test for the new case (we're not throwing an exception anymore)

Comment on attachment 697153 [details] [diff] [review] add webidl GetBoundingClientRect() Review of attachment 697153 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsRange.cpp @@ +2596,2 @@ > if (!mStartParent) > + return nullptr; You're changing behaviour here; we always returned a non-null rect before.

Comment on attachment 697168 [details] [diff] [review] add IntersectsNode() webidl version Review of attachment 697168 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsRange.cpp @@ +757,5 @@ > { > *aResult = false; > > nsCOMPtr<nsINode> node = do_QueryInterface(aNode); > // TODO: This should throw a TypeError. Remove this todo

Comment on attachment 697168 [details] [diff] [review] add IntersectsNode() webidl version I don't see need for removing the todo ;)

Comment on attachment 697178 [details] [diff] [review] add SetStart() webidl version >+nsRange::SetStart(nsINode& aNode, uint32_t aOffset, ErrorResult& rv) aRv >+{ >+ if (!nsContentUtils::CanCallerAccess(&aNode)) { >+ rv.Throw(NS_ERROR_DOM_SECURITY_ERR); >+ return; >+ } Could you please file a followup to sort out CanCallerAccess handling. I believe CanCallerAccess isn't needed at all in Range code.

Comment on attachment 697190 [details] [diff] [review] add nsINode version of IndexOf() > static int32_t >+IndexOf(nsINode* aChild) >+{ >+ nsINode *parent = aChild->GetParentNode(); nsINode* parent

Comment on attachment 697304 [details] [diff] [review] add webidl SetEnd() >+void >+nsRange::SetEnd(nsINode& aNode, uint32_t aOffset, ErrorResult& rv) aRv

Comment on attachment 697305 [details] [diff] [review] add webidl SetEnd After / Before() >+nsRange::SetEndBefore(nsINode& aNode, ErrorResult& rv) aRv >+nsRange::SetEndAfter(nsINode& aNode, ErrorResult& rv) aRv

Comment on attachment 697306 [details] [diff] [review] add SelectNode() webidl version >+nsRange::SelectNode(nsINode& aNode, ErrorResult& rv) aRv

Comment on attachment 697307 [details] [diff] [review] add webidl SelectNodeContents() >+void >+nsRange::SelectNodeContents(nsINode& aNode, ErrorResult& rv) aRv

Comment on attachment 697476 [details] [diff] [review] add CompareBoundaryPoints() webidl version >+nsRange::CompareBoundaryPoints(uint16_t aHow, nsRange& aOtherRange, >+ ErrorResult& rv)

(In reply to :Ms2ger from comment #18) > Comment on attachment 697153 [details] [diff] [review] > add webidl GetBoundingClientRect() > > Review of attachment 697153 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsRange.cpp > @@ +2596,2 @@ > > if (!mStartParent) > > + return nullptr; > > You're changing behaviour here; we always returned a non-null rect before. no, we were not. We used to have *aResult = nullptr; if (!mStartParent) return NS_OK; so if mStartParent was null (is that possible anyway) we set *aReturn to null and returned NS_Ok.

(In reply to Trevor Saunders (:tbsaunde) from comment #29) > (In reply to :Ms2ger from comment #18) > > Comment on attachment 697153 [details] [diff] [review] > > add webidl GetBoundingClientRect() > > > > Review of attachment 697153 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > ::: content/base/src/nsRange.cpp > > @@ +2596,2 @@ > > > if (!mStartParent) > > > + return nullptr; > > > > You're changing behaviour here; we always returned a non-null rect before. > > no, we were not. We used to have > > *aResult = nullptr; > if (!mStartParent) > return NS_OK; > > so if mStartParent was null (is that possible anyway) we set *aReturn to > null and returned NS_Ok. Quoting directly: *aResult = nullptr; // Weak ref, since we addref it below nsClientRect* rect = new nsClientRect(); if (!rect) return NS_ERROR_OUT_OF_MEMORY; NS_ADDREF(*aResult = rect); if (!mStartParent) return NS_OK;

(In reply to :Ms2ger from comment #30) > (In reply to Trevor Saunders (:tbsaunde) from comment #29) > > (In reply to :Ms2ger from comment #18) > > > Comment on attachment 697153 [details] [diff] [review] > > > add webidl GetBoundingClientRect() > > > > > > Review of attachment 697153 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > ::: content/base/src/nsRange.cpp > > > @@ +2596,2 @@ > > > > if (!mStartParent) > > > > + return nullptr; > > > > > > You're changing behaviour here; we always returned a non-null rect before. > > > > no, we were not. We used to have > > > > *aResult = nullptr; > > if (!mStartParent) > > return NS_OK; > > > > so if mStartParent was null (is that possible anyway) we set *aReturn to > > null and returned NS_Ok. > > Quoting directly: yeah, your right I was looking at GetClientRects() which behaves differently...

Comment on attachment 697154 [details] [diff] [review] add webidl GetClientRects() r=me

Comment on attachment 697174 [details] [diff] [review] add CommonAncestorContainer() This doesn't look right to me. In the old setup, we'd throw if GetCommonAncestor returned null, while in the new one we'll crash on a null deref, looks like.

Comment on attachment 697176 [details] [diff] [review] add nsContentUtils::CanCallerAccess(nsINode*) r=me

Comment on attachment 697303 [details] [diff] [review] add SetStart Before / After() webidl version This changes behavior to crash instead of throwing when SetStartBefore/After are called on a node with no parent... I don't think that's desirable.

Comment on attachment 697702 [details] [diff] [review] bug 825341 - add webidl GetBoundingClientRect() Why does this need an ErrorResult? I don't think it does... r=me with that removed.

(In reply to Boris Zbarsky (:bz) from comment #36) > Comment on attachment 697303 [details] [diff] [review] > add SetStart Before / After() webidl version > > This changes behavior to crash instead of throwing when SetStartBefore/After > are called on a node with no parent... I don't think that's desirable. I don't think it does, it calls SetStart(nsINode*, int32_t) which calls IsValidBoundary() which null checks, and if that fails throws.

Comment on attachment 697303 [details] [diff] [review] add SetStart Before / After() webidl version Hmm. Indeed. That's worth documenting, since it's non-obvious....

(In reply to Olli Pettay [:smaug] from comment #17) > Comment on attachment 697168 [details] [diff] [review] > add IntersectsNode() webidl version > > > > > >- NS_ENSURE_TRUE(mIsPositioned, NS_ERROR_NOT_INITIALIZED); > >+bool > >+nsRange::IntersectsNode(nsINode& aNode) > >+{ > >+ if (!mIsPositioned) { > >+ return false; > >+ } > > So you actually change the behavior here. > Please add a test for the new case (we're not throwing an exception anymore) hm, atually that change looks either wrong or like something that should go in its own bug no?

Comment on attachment 698481 [details] [diff] [review] bug 825341 - add webidl InsertNode() > // Now actually insert the node > nsCOMPtr<nsIDOMNode> tResultNode; >- res = referenceParentNode->InsertBefore(aNode, referenceNode, getter_AddRefs(tResultNode)); >- NS_ENSURE_SUCCESS(res, res); >+ nsCOMPtr<nsIDOMNode> node = do_QueryInterface(&aNode); >+ if (!node) { >+ aRv.Throw(NS_ERROR_DOM_NOT_OBJECT_ERR); >+ return; >+ } No need for the QI. nsINode has AsDOMNode() (Would be ofc nice to just use nsINode API, but if it causes more changes, no need to do it in this bug)

Comment on attachment 698485 [details] [diff] [review] make nsIDocumentFragment [builtinclass] Make sure to push this to try.

Comment on attachment 698486 [details] [diff] [review] make CutContents() take a DocumentFragment** and add webidl ExtractContents() >- nsresult CutContents(nsIDOMDocumentFragment** frag); >+ nsresult CutContents(mozilla::dom::DocumentFragment** frag); While you're here s/frag/aFrag/

Comment on attachment 698498 [details] [diff] [review] add CloneContents() webidl version >+ if (!commonCloneAncestor) { >+ aRv.Throw(NS_ERROR_FAILURE); >+ return nullptr; >+ } You could just MOZ_ASSERT here.

Comment on attachment 698499 [details] [diff] [review] add webidl SurroundNode() >- res = aNewParent->AppendChild(docFrag, getter_AddRefs(tmpNode)); >- if (NS_FAILED(res)) return res; >+ nsCOMPtr<nsIDOMNode> parent = do_QueryInterface(&aNewParent); >+ if (!parent) { >+ aRv.Throw(NS_ERROR_DOM_NOT_OBJECT_ERR); >+ return; >+ } >+ >+ aRv = parent->AppendChild(docFrag, getter_AddRefs(tmpNode)); aNewParent->AsDOMNode->AppendChild(... (unless using nsINode::AppendChild works)

Comment on attachment 698525 [details] [diff] [review] add nsContentUtils ContextualFragment() with dom::DocumentFragment** >+ DocumentFragment* frag; Please initialize to nullptr Otherwise you might return pointer to random value. >+#define NS_I_FRAGMENT_CONTENT_SINK_IID \ >+ {0xd44775e0, 0xea21, 0x477f, \ >+ {0xb4, 0x1d, 0x92, 0x29, 0x70, 0x9d, 0x17, 0xb1}} > > /** > * The fragment sink allows a client to parse a fragment of sink, possibly > * surrounded in context. Also see nsIParser::ParseFragment(). > * Note: once you've parsed a fragment, the fragment sink must be re-set on > * the parser in order to parse another fragment. > */ > class nsIFragmentContentSink : public nsISupports { > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_I_FRAGMENT_CONTENT_SINK_IID) > /** > * This method is used to obtain the fragment created by > * a fragment content sink and to release resources held by the parser. > * > * The sink drops its reference to the fragment. > */ >- NS_IMETHOD FinishFragmentParsing(nsIDOMDocumentFragment** aFragment) = 0; >+ NS_IMETHOD FinishFragmentParsing(mozilla::dom::DocumentFragment** aFragment) = 0; Hmm, make sure this isn't used outside libxul. Or I'd prefer to just not change this in this bug.

> >- NS_IMETHOD FinishFragmentParsing(nsIDOMDocumentFragment** aFragment) = 0; > >+ NS_IMETHOD FinishFragmentParsing(mozilla::dom::DocumentFragment** aFragment) = 0; > > Hmm, make sure this isn't used outside libxul. Or I'd prefer to just not > change this in this bug. Well, we can't really know what binary addons do... Though keeping this the same and static casting the result might be the better approach anyway.

mOwner in GetParentObject() was agreed on with smaug / peterv

I don't have muchh to say for this patch other than it seems to build and pass dom/imptests/

Comment on attachment 699909 [details] [diff] [review] add mOwner member to nsRange and have constructor init it to current ownerDoc > already_AddRefed<nsRange> > nsRange::CloneRange() const > { >- nsRefPtr<nsRange> range = new nsRange(); >+ nsRefPtr<nsRange> range = new nsRange(mStartParent); mOwner >+ nsRange(nsINode* aNode) >+ : mOwner(nullptr) >+ , mRoot(nullptr) > , mStartOffset(0) > , mEndOffset(0) > , mIsPositioned(false) > , mIsDetached(false) > , mMaySpanAnonymousSubtrees(false) > , mInSelection(false) > , mStartOffsetWasIncremented(false) > , mEndOffsetWasIncremented(false) > #ifdef DEBUG > , mAssertNextInsertOrAppendIndex(-1) > , mAssertNextInsertOrAppendNode(nullptr) > #endif >- {} >+ { >+ if (aNode) { >+ mOwner = aNode->OwnerDoc(); >+ } >+ } I think we should MOZ_ASSERT that aNode isn't null. That means creating ranges using createInstance won't be possible, but one should always use nsIDOMDocument::createRange in external code, IMO. > static nsresult CreateRange(nsIDOMNode* aStartParent, int32_t aStartOffset, > nsIDOMNode* aEndParent, int32_t aEndOffset, > nsRange** aRange); > static nsresult CreateRange(nsIDOMNode* aStartParent, int32_t aStartOffset, > nsIDOMNode* aEndParent, int32_t aEndOffset, > nsIDOMRange** aRange); >@@ -263,17 +269,18 @@ protected: > ~AutoInvalidateSelection(); > nsRange* mRange; > nsRefPtr<nsINode> mCommonAncestor; > #ifdef DEBUG > bool mWasInSelection; > #endif > static bool mIsNested; > }; >- >+ >+ nsCOMPtr<nsIDocument> mOwner; Missing space? You need to add mOwner to cycle collection

(In reply to Olli Pettay [:smaug] from comment #65) > Comment on attachment 699909 [details] [diff] [review] > add mOwner member to nsRange and have constructor init it to current ownerDoc > > > already_AddRefed<nsRange> > > nsRange::CloneRange() const > > { > >- nsRefPtr<nsRange> range = new nsRange(); > >+ nsRefPtr<nsRange> range = new nsRange(mStartParent); > mOwner ok > >+ nsRange(nsINode* aNode) > >+ : mOwner(nullptr) > >+ , mRoot(nullptr) > > , mStartOffset(0) > > , mEndOffset(0) > > , mIsPositioned(false) > > , mIsDetached(false) > > , mMaySpanAnonymousSubtrees(false) > > , mInSelection(false) > > , mStartOffsetWasIncremented(false) > > , mEndOffsetWasIncremented(false) > > #ifdef DEBUG > > , mAssertNextInsertOrAppendIndex(-1) > > , mAssertNextInsertOrAppendNode(nullptr) > > #endif > >- {} > >+ { > >+ if (aNode) { > >+ mOwner = aNode->OwnerDoc(); > >+ } > >+ } > I think we should MOZ_ASSERT that aNode isn't null. > That means creating ranges using createInstance won't be possible, but one > should always use > nsIDOMDocument::createRange in external code, IMO. ok, the reason I didn't do that fwiw is that I'm not really sure all the callers will actually never pass NULL. > >+ nsCOMPtr<nsIDocument> mOwner; > Missing space? > > You need to add mOwner to cycle collection yeah, how did I forget? :(

(In reply to Trevor Saunders (:tbsaunde) from comment #66) > ok, the reason I didn't do that fwiw is that I'm not really sure all the > callers will actually never pass NULL. Well we need to make sure range objects are initialized reasonable way. So either ensure that ctor isn't called with null or add Init(nsINode* aOwner) which throws if null is passed.

Comment on attachment 699526 [details] [diff] [review] bug 825341 - add CommonAncestorContainer() >+ if (commonAncestor) { >+ NS_ADDREF(*aCommonParent = commonAncestor->AsDOMNode()); Else set *aCommonParent to null. r=me with that.

Comment on attachment 699530 [details] [diff] [review] rest of range api Can use call the WebIDL CutContents, please? Or is there not one? r=me

Comment on attachment 699902 [details] [diff] [review] use webidl for Range While you're here, fix the indentation in the nsRange constructor? >+++ b/content/test/unit/test_range.js Why remove the tests instead of fixing them? >+++ b/dom/imptests/testharnessreport.js Please have ms2ger review this part. >+/*}; We support "partial interface" now. No need for the comment hack. r=me with those fixed.

Comment on attachment 699902 [details] [diff] [review] use webidl for Range Review of attachment 699902 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/imptests/testharnessreport.js @@ +14,5 @@ > > /** > * If set to true, we will dump the test failures to the console. > */ > + "dumpFailures": true, Don't forget to revert this part :)

> >+++ b/content/test/unit/test_range.js > > Why remove the tests instead of fixing them? because dom/imptests/ should cover the same thing I believe, though I guess we might want to keep them around because tests outside of dom/imptests/ can be easier to debug. > >+/*}; > > We support "partial interface" now. No need for the comment hack. yeah, but we didn't when the patch was written ;)

> because dom/imptests/ should cover the same thing I believe I wouldn't trust imptests too much. ;) Just fix our tests too. > yeah, but we didn't when the patch was written ;) Sure. Just saying we shouldn't land the patch as-is.

(In reply to Boris Zbarsky (:bz) from comment #73) > > because dom/imptests/ should cover the same thing I believe > > I wouldn't trust imptests too much. ;) Just fix our tests too. ack > Can use call the WebIDL CutContents, please? Or is there not one? there isn't one, its just a helper, though yeah it may make sense to make it more webidlish at some point.

ugh, meant to post this a while ago but forgot :(

Comment on attachment 703009 [details] [diff] [review] bug 825341 - add mOwner member to nsRange and have constructor init it to current ownerDoc > nsRange::CreateRange(nsIDOMNode* aStartParent, int32_t aStartOffset, > nsIDOMNode* aEndParent, int32_t aEndOffset, > nsRange** aRange) > { > MOZ_ASSERT(aRange); > *aRange = NULL; > I think this could have NS_ENSURE_STATE(aStartParent); >+ nsRange(nsINode* aNode) >+ : mOwner(nullptr) No need to assign null to nsCOMPtr >+ MOZ_ASSERT(aNode, "range isn't in a document!"); >+ if (aNode) { >+ mOwner = aNode->OwnerDoc(); >+ } Don't add the null check. If we want ranges to work consistently, they just must have mOwner always. > nsTypeAheadFind::Init(nsIDocShell* aDocShell) > { > nsCOMPtr<nsIPrefBranch> prefInternal(do_GetService(NS_PREFSERVICE_CONTRACTID)); >- mSearchRange = new nsRange(); >- mStartPointRange = new nsRange(); >- mEndPointRange = new nsRange(); >+ nsIPresShell* presShell = aDocShell->GetPresShell(); GetPresShell may return null > nsCOMPtr<nsIPresShell> presShell; > presShell = aDocShell->GetPresShell(); > mPresShell = do_GetWeakReference(presShell); > > mStartFindRange = nullptr; >- mStartPointRange = new nsRange(); >- mSearchRange = new nsRange(); >- mEndPointRange = new nsRange(); >+ mStartPointRange = new nsRange(presShell->GetDocument()); >+ mSearchRange = new nsRange(presShell->GetDocument()); >+ mEndPointRange = new nsRange(presShell->GetDocument()); What guarantees that presShel isn't null here? Could you still go through once the places where new Range() is called and make sure null isn't passed. Or is this too error prone? Should we allow null mOwner, but in such cases throw whenever someone tries to actually use the Range object? I think we should at least try to enforce non-null mOwner, and only if that doesn't work out, then try the other approach.

(In reply to Olli Pettay [:smaug] from comment #76) > Comment on attachment 703009 [details] [diff] [review] > bug 825341 - add mOwner member to nsRange and have constructor init it to > current ownerDoc > > > > nsRange::CreateRange(nsIDOMNode* aStartParent, int32_t aStartOffset, > > nsIDOMNode* aEndParent, int32_t aEndOffset, > > nsRange** aRange) > > { > > MOZ_ASSERT(aRange); > > *aRange = NULL; > > > I think this could have NS_ENSURE_STATE(aStartParent); meaning you think this should throw instead of not allow null aStartParent? > >+ MOZ_ASSERT(aNode, "range isn't in a document!"); > >+ if (aNode) { > >+ mOwner = aNode->OwnerDoc(); > >+ } > Don't add the null check. If we want ranges to work consistently, they just > must have mOwner always. > > > nsTypeAheadFind::Init(nsIDocShell* aDocShell) > > { > > nsCOMPtr<nsIPrefBranch> prefInternal(do_GetService(NS_PREFSERVICE_CONTRACTID)); > >- mSearchRange = new nsRange(); > >- mStartPointRange = new nsRange(); > >- mEndPointRange = new nsRange(); > >+ nsIPresShell* presShell = aDocShell->GetPresShell(); > GetPresShell may return null ok, what do we pass to nsRange() if it does? > > nsCOMPtr<nsIPresShell> presShell; > > presShell = aDocShell->GetPresShell(); > > mPresShell = do_GetWeakReference(presShell); > > > > mStartFindRange = nullptr; > >- mStartPointRange = new nsRange(); > >- mSearchRange = new nsRange(); > >- mEndPointRange = new nsRange(); > >+ mStartPointRange = new nsRange(presShell->GetDocument()); > >+ mSearchRange = new nsRange(presShell->GetDocument()); > >+ mEndPointRange = new nsRange(presShell->GetDocument()); > What guarantees that presShel isn't null here? this is the same issue as the last comment right? it could be null, but we need to come up with something to use as the owner if it is. > Could you still go through once the places where new Range() is called and > make sure null isn't passed. The problem I had with being sure nul is never passed when I wrote this was that there are all sorts of things where it might be null, but I have no idea how to handle them (the typeaheadfind case discussed above is one of the biggest concerns I ad). > Or is this too error prone? Should we allow null mOwner, but in such cases > throw whenever someone tries to actually use the Range object? if we were to do this why not just "abuse" mIsPositioned to do this? when range changes from not positioned to positioned we must have node, so we can give it sane owner, and we'll already throw if range isn't positioned. > I think we should at least try to enforce non-null mOwner, and only if that > doesn't work out, then try the other approach. I certainly agree mOwner never being null is the final state we want. however I don't know how to make that work right now.

(In reply to Trevor Saunders (:tbsaunde) from comment #77) > meaning you think this should throw instead of not allow null aStartParent? Meaning that mOwner is never null > ok, what do we pass to nsRange() if it does? Init should fail? > > this is the same issue as the last comment right? it could be null, but we > need to come up with something to use as the owner if it is. We need to return early? Or just make sure the methods aren't ever called with null docshell which has null presshell? > if we were to do this why not just "abuse" mIsPositioned to do this? when > range changes from not positioned to positioned we must have node, so we can > give it sane owner, and we'll already throw if range isn't positioned. Well, technically that may lead to somewhat random mOwner, which is not good. Could we make WrapObject to fail if nsRange doesn't have mOwner or something like that to ensure that JS can't access nsRange objects without mOwner

Comment on attachment 703649 [details] [diff] [review] bug 825341 - add mOwner member to nsRange and have constructor init it to current ownerDoc > nsRange::CreateRange(nsIDOMNode* aStartParent, int32_t aStartOffset, > nsIDOMNode* aEndParent, int32_t aEndOffset, > nsRange** aRange) > { >+ NS_ENSURE_ARG_POINTER(aStartParent); >+ NS_ENSURE_ARG_POINTER(aEndParent); No reason for these. aStartParent is null-checked just below and aEndParent in SetEnd > already_AddRefed<nsRange> > nsRange::CloneRange() const > { >- nsRefPtr<nsRange> range = new nsRange(); >+ nsRefPtr<nsRange> range = new nsRange(mOwner); To be super safe, there should be null check for mOwner (it will be null after unlink). >@@ -255,17 +257,22 @@ nsFindContentIterator::Reset() > nsCOMPtr<nsIContent> endContent(do_QueryInterface(mEndNode)); > if (endContent) { > mEndOuterContent = endContent->FindFirstNonChromeOnlyAccessContent(); > } > > // Note: OK to just set up the outer iterator here; if our range has a native > // anonymous endpoint we'll end up setting up an inner iterator, and > // reset the outer one in the process. >- nsCOMPtr<nsIDOMRange> range = nsFind::CreateRange(); >+ nsCOMPtr<nsINode> node = do_QueryInterface(mStartNode); >+ NS_ASSERTION(node, "iterator without a start node?"); >+ if (!node) >+ return; Useless assertion before null check. Also if (expr) { stmt; } Though, we do have _VOID versions of NS_ENSURE_* macros

needed because browser.js doesn't deal with SetDocShell() failing so we need to always have a doc to create the range with https://tbpl.mozilla.org/?tree=Try&rev=29edf629cb6a

Comment on attachment 704636 [details] [diff] [review] bug 825341 - fixup taf you need to still nullcheck doc.

filed 839010 for the CanCallerAccess() stuff

so it doesn't leak

we need to not fail when just GetRoot() returns null because then test_bug629845.html fails because the editor isn't created.

Comment on attachment 711846 [details] [diff] [review] bug 825341 - fixup taf >@@ -624,16 +624,20 @@ nsTypeAheadFind::FindItNow(nsIPresShell *aPresShell, bool aIsLinksOnly, > continue; > } > > if (aFindPrev) { > // Reverse mode: swap start and end points, so that we start > // at end of document and go to beginning > nsCOMPtr<nsIDOMRange> tempRange; > mStartPointRange->CloneRange(getter_AddRefs(tempRange)); >+ if (!mEndPointRange) { >+ mEndPointRange = new nsRange(presShell->GetDocument()); >+ } >+ You should have non-null document in this context. It is the same as presShell->GetDocument() This is a bit fragile.

I wonder if we should wait until the next merge, so that this rather large change is landed early in a cycle.

Unfortunately, this had to be backed out due to causing perma-orange in the Linux32 debug mochitest-2 suite. https://hg.mozilla.org/integration/mozilla-inbound/rev/ca34c11bf55d https://tbpl.mozilla.org/php/getParsedLog.php?id=19996178&tree=Mozilla-Inbound 11:40:07 INFO - 7414 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | 58,5,2: context range 58 [foreignDocfrag, 0, foreignDocfrag, 0], argument range 5 [paras[1].firstChild, 0, paras[1].firstChild, 0], how 2 11:40:07 INFO - 7415 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | Elided 100 passes or known failures. 11:40:07 INFO - 7416 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | 58,30,2: context range 58 [foreignDocfrag, 0, foreignDocfrag, 0], argument range 30 [paras[0], 0, paras[0].firstChild, 7], how 2 11:40:07 INFO - 7417 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | Elided 100 passes or known failures. 11:40:07 INFO - 7418 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | 58,55,2: context range 58 [foreignDocfrag, 0, foreignDocfrag, 0], argument range 55 [detachedForeignComment, 4, detachedForeignComment, 4], how 2 11:40:07 INFO - 7419 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | Elided 100 passes or known failures. 11:40:07 INFO - 7420 INFO TEST-PASS | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | 59,19,2: context range 59 [xmlDocfrag, 0, xmlDocfrag, 0], argument range 19 [document.body, 4, document.body, 5], how 2 11:40:07 INFO - Assertion failure: !cx->isExceptionPending(), at ../../../js/src/jsinterp.cpp:3343 11:40:08 WARNING - TEST-UNEXPECTED-FAIL | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | Exited with code 11 during test run 11:40:08 WARNING - This is a harness error. 11:40:14 INFO - PROCESS-CRASH | /tests/dom/imptests/webapps/DOMCore/tests/approved/test_Range-compareBoundaryPoints.html | application crashed [@ js::Throw(JSContext*, JS::Handle<JS::Value>)] 11:40:14 INFO - Crash dump filename: /tmp/tmp9kXaAe/minidumps/0aed8c72-5e01-700b-4b2c6c02-663ebcf2.dmp 11:40:14 INFO - Operating system: Linux 11:40:14 INFO - 0.0.0 Linux 2.6.31.5-127.fc12.i686.PAE #1 SMP Sat Nov 7 21:25:57 EST 2009 i686 11:40:14 INFO - CPU: x86 11:40:14 INFO - GenuineIntel family 6 model 23 stepping 10 11:40:14 INFO - 2 CPUs 11:40:14 INFO - Crash reason: SIGSEGV 11:40:14 INFO - Crash address: 0x0 11:40:14 INFO - Thread 0 (crashed) 11:40:14 INFO - 0 libxul.so!js::Throw(JSContext*, JS::Handle<JS::Value>) [jsinterp.cpp:24f601f285dc : 3343 + 0x0] 11:40:14 INFO - eip = 0x02c57f6b esp = 0xbfcf83b0 ebp = 0xbfcf83e8 ebx = 0x037ae688 11:40:14 INFO - esi = 0x00c59844 edi = 0xa5462d00 eax = 0x00000000 ecx = 0x00c5a32c 11:40:14 INFO - edx = 0x00000000 efl = 0x00010246 11:40:14 INFO - Found by: given as instruction pointer in context 11:40:14 INFO - 1 0x40f5004 11:40:14 INFO - eip = 0x040f5005 esp = 0xbfcf83f0 ebp = 0x99e099e0 11:40:14 INFO - Found by: previous frame's frame pointer 11:40:14 INFO - 2 libxul.so!js::ion::InvokeFunction(JSContext*, JS::Handle<JSFunction*>, unsigned int, JS::Value*, JS::Value*) [VMFunctions.cpp:24f601f285dc : 53 + 0xa] 11:40:14 INFO - eip = 0x0306ec0b esp = 0xbfcf83fc ebp = 0x99e099e0 11:40:14 INFO - Found by: stack scanning

getting a setup to debug on a test slave really sucks, and I think m2 on linux32 magically became green when I psuhed to try over the weekend, so pushed to inbound with fingers crossed https://hg.mozilla.org/integration/mozilla-inbound/rev/2c8930343985

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a40c76947487 for having probably needed-clobber - there are clobbered retriggers building in https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=fee79a593fd3, so if the second time around comes up clean, touch /CLOBBER and https://secure.pub.build.mozilla.org/clobberer/?branch=mozilla-inbound and land again.

Fedora 32bit debug was a free space clobber and has the same test failures as all the other Mbc tests.

Yeah, it was the Win debug 1/4/oth that I was expecting to be needs-clobber. The bc... goodness gracious, is some social test our only test of typeaheadfind?

And indeed, the 1/4/oth crashes in mozilla::dom::InterfaceHasInstance went away on a clobber.

(In reply to Phil Ringnalda (:philor) from comment #93) > Yeah, it was the Win debug 1/4/oth that I was expecting to be needs-clobber. so everything but b-c went green? I'm not sure what your starring means. > The bc... goodness gracious, is some social test our only test of > typeaheadfind? no, it means that browser.xml is probably crazy and forces a typeaheadfind object to be allocated on page hide if it doesn't already have one.

Right, there were two failures, browser-chrome "(NS_ERROR_UNEXPECTED) [nsITypeAheadFind.init]" in browser_social_activation.js on every platform, which was still happening after a clobber, and, mozilla::dom::InterfaceHasInstance crashes on Windows debug mochitest-1, mochitest-4 and mochitest-other, which went away after a clobber.

the typeahead thing was trivial browser.xml forces taf creation on page hide, and that meant taf::Init() couldn't get a pres shell, but it didn't use the pres shell for anything so I just got rid of that remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/5410bada8645

The windows clobber thing is likely bug 850153.