469020 - range.surroundContents throws a NS_ERROR_INVALID_POINTER (code works on FF 3.0)

Status: RESOLVED FIXED

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

Description

[Elia]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.4) Gecko/2008102920 Firefox/3.0.4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2) Gecko/20081201 Firefox/3.1b2

Generally I'd check my code for a longer time before submitting a bug but this time I decided to make an exception in that 3.1 is still in beta version and could possibly be a bug as the code works perfectly in Firefox 3.0 and maybe by reporting it (in the case that this turns out not to be a bug) we can bring this issue up so that other people using this type of code can see and be notified of a possible API change (although I highly doubt there are actually other crazy enough people to use this exact kind of code).

Now, the problem here is that it's almost impossible to create a test case because I'd have to determine what code causes this specifically (as the range gets created using a lot of code) and it'd be easier to just put the current version of the extension on my website plus the page I'm testing the add-on on to investigate the issue, which I've done.

You can get the extension here: http://www.quieturl.com/get/1.6.0.5/
and the test page is this one: http://www.quieturl.com/urls.html

Note that 1.6.0.5 is a temporary fix version. 

Note also that although as I said I didn't spend a lot of time checking this before reporting I did some testing to determine whether this was unexpected behavior or not. 
First of all, I checked with a debugger that the ANCHOR element I pass to surroundContents is OK (ie, not null and valid) then I did the same thing for the range object I call surroundContents on and double checked that it's valid and not partially selecting non-text elements as the standard says and anyway in this case a BAD_BOUNDARYPOINTS_ERR exception should be raised and not an NS_ERROR_INVALID_POINTER so would still need fixing (I see there was another bug complaining that exceptions were not thrown in this case but that bug was very old and fixed and was talking about NO exceptions being thrown, not just the wrong one).

Reproducible: Always

Steps to Reproduce:
1. Install the add-on
2. Go to the page specified above
3. Hover with the mouse on the first url (anchor created)
3. Hover with the mouse on the second url (anchor created)
3. Hover with the mouse on the third url (anchor created)
3. Hover again with the mouse on the second url
4. You get an NS_ERROR_INVALID_POINTER exception (check in error console)
Actual Results:  
You get an NS_ERROR_INVALID_POINTER error (check in error console) and the range doesn't get correctly surrounded.

Expected Results:  
The range should be surrounded by an anchor.

Comment 1

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

Any chance for a *minimal* testcase? It would help debugging a lot, since
I know nothing about your extension.

Comment 2

[Elia]

Uhm, as I said, that's not easy. I'll try anyway to create something workable. 
In the meantime if you have knowledge about range-related code do you know in what cases the NS_ERROR_INVALID_POINTER exception is thrown?

Comment 3

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

Because of the error, bug 454326 didn't probably cause this.
Bug 332148 might have.

But still, need to debug this first to see if this is a valid bug, or did some
bug fix change our behavior so that we follow the spec correctly and the
testcase expects the old (wrong) behavior.
As I said, *minimal* testcase would be really really useful here.

Comment 4

[Elia]

Yes I already noticed the bug about extractContents (it was listed as one of the modifications for 3.1) I forgot to mention it.

And I'm trying to create a small test case, hope to get it working soon (will updated this when done).

Comment 5

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

Bug 454325 might have caused this, at least it changes the code so that
CloneParentsBetween is called (via extracContents) when surroundContents is called, and CloneParentsBetween may return NS_ERROR_INVALID_POINTER.

Comment 6

[Elia]

Hum I've noticed another weird thing (or possibly this is the cause in the first place, although doesn't make a lot of sense).
When you use range.extractContents seems like if the parent node remains empty (I guess) it is removed from the document but NOT returned in the doc fragment which I don't think it's how it should behave, am I wrong? Didn't read about this in the standard at least.
See it here: http://www.quieturl.com/testurls.html

Press DO RANGE and then UNDO RANGE. You get first the extracted contents from the anchor node and then you should get a message "Anchor still present" if the anchor is still in its parent node. Check out the code for more info.

Comment 7

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

(In reply to comment #6)
You remove anchor from its parent when you do var contents = range.extractContents(); and in the loop you get the innerHTML of <a>.
So I don't see anything wrong there. 

I'm not sure why you have range2. You're not using it for anything.
If you use range2 to extractContents, then <a> isn't removed from its parent, because range2 selects only contents of <a>, not <a> itself.

Comment 8

[Elia]

Yes I overlooked the code a bit I was in a hurry, which is bad. The code should have been range2.exctractContents. With the corrected code the "Anchor is still present" message appears, the problem was that in my code it didn't work probably because of the exception being thrown (NS_ERROR_INVALID_POINTER), although that's strange as it is in a catch statement.
Nevertheless the problem regarding NS_ERROR_INVALID_POINTER is still present and I was trying to create a test case for that when I stumbled upon this one about the replaceChild not working. I'll try to create a valid test case for the original problem now although it seems a little hard to do as my range code is complex and not easily reproducible.

Comment 9

[Elia]

I was finally able to build up a test case.
What I've found is that my code initially (when it gets the range composing the second url) creates a range with 5 elements in it (which is fine as there are 5 element siblings within the paragraph) with startContainer of "http://www." and startOffset of 0 which is perfect because it's exactly the start of the url and when I do the surroundContents the first time it works fine indeed.
But when I call the surroundContents something happens because the second time I call it the range starts with an empty text node _adjacent_ to the old "http://www." text node. 
Uploading the test case after this message. Just press DO RANGE and then UNDO RANGE and then again DO RANGE. The code always set the first child of the paragraph to be the startContainer and startOffset of 0 and endContainer to be the last child of the paragraph with endOffset to the length of the last child nodeValue.
I guess this could be avoided by checking whether startContainer or endContainer are empty text nodes and in that case skip to the next/previous node, correcting the offset, but I don't know if not doing this should really throw an exception (I don't really see the reason why an empty text node can not be the boundary of a range, but maybe I'm wrong).

Comment 10

[Elia]

Attachment: Test case for range empty text nodes boundary problem

Comment 11

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

The empty nodes are ok here, because you use textnodes+extractContents (via surroundContents).
But still, the second time "DO RANGE" is called, it should be possible to add <a> around the selected content.

As a workaround you could either not select empty textnodes, or you could call
normalize() after surroundContents() or extractContents()

Comment 12

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

btw, why you have
range.selectNodeContents(elem);
and			
range.setStart(elem.firstChild,0);
range.setEnd(elem.lastChild,elem.lastChild.length);

First you select in one way, then in some other way ;)

Comment 13

[Elia]

Smaug: and so? What's the problem in that code? Also, if you comment out the range.selectNodeContents(elem); the error is the same, so I don't think it really matters.

Comment 14

[Elia]

Sorry I didn't read the "why". Well I just added the lines of code to mimic the range that was created in my code. In my experience calling it before the other lines doesn't cause any problem and indeed as I said the error is the same. I'll try the normalize function to see if it helps, thanks for the tip.

Comment 15

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

Anyway, I have the fix for this. I just need to write mochitests..

Comment 16

[Elia]

Nice, normalize did the trick. See, you're never documented enough. I could have come up to this workaround on my own without wasting other people's time. Well, I hope at least this would be a useful bug report and/or reference for other users with this issue.

Comment 17

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

This is very useful bug report, and the fix is coming soon... :)

Comment 18

[Elia]

Yes I uploaded the add-on's new version anyway as I figured out using normalize() is a good practice anyway and should not compromise the functionality at all once the bug gets fixed on firefox 3.1 final.

Comment 19

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

Attachment: proposed patch + mochitests

Safari and Opera pass the mochitest too.

Comment 20

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

So the problem is that we don't always have the nodeToResult when we should.

Comment 21

[Mike Beltzner [:beltzner, not reading bugmail]]

Can I get the hg reference for when/where this landed on trunk, please?

Comment 22

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

This was fixed 2008-12-13 (as bug history tells).
http://hg.mozilla.org/mozilla-central/rev/4e478dbf4948

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 352619 [details] [diff] [review]
proposed patch + mochitests

a191=beltzner