526939 - DOM Inspector hangs trying to find a ID that doesn't exist

Status: RESOLVED FIXED

(Other Applications :: DOM Inspector, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Open DOM Inspector.
2. Cmd+F
3. Type something that isn't an ID in the document.
4. Press Enter.

Result: DOM Inspector hangs, even on short documents.

Expected: Tell me 'not found', preferably without using a nested dialog.


Tested with:
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091103 Firefox/3.7a1pre

Happens with both:
DOM Inspector 2.0.3
DOM Inspector 2.0.4 (AMO sandbox)

Comment 1

[Shawn Wilsher :sdwilsh]

Do you have a stack of the hang?

Comment 2

[Jesse Ruderman]

Attachment: stack from domi 2.0.3

Comment 3

[Colby Russell :crussell (no longer Mozilla-ing)]

I get the same thing on latest Ubuntu. Hanging occurs on XULRunner 1.9.2 while the same app using 1.9.1 works fine.

Comment 4

[Colby Russell :crussell (no longer Mozilla-ing)]

I traced it back to improper behavior for inDeepTreeWalker::nextNode, where it will just sit on the last node if the end of the document is reached, instead of setting its currentNode to null. Seems like it has something to do with the rewrite in bug 522601.

Comment 5

[Boris Zbarsky [:bzbarsky]]

> improper behavior for inDeepTreeWalker::nextNode, where it will just sit on the
> last node if the end of the document is reached, instead of setting its
> currentNode to null

That's the correct behavior of a DOM TreeWalker.  The DOM spec is very clear on this issue:

  nextNode
    Moves the TreeWalker to the next visible node in document order relative to
    the current node, and returns the new node. If the current node has no next
    node, or if the search for nextNode attempts to step upward from the
    TreeWalker's root node, returns null, and retains the current node. 

So the real issue is that I missed an inDOMTreeWalker consumer that was depending on the old, buggy, behavior...

Colby, did you digging turn up the relevant consumer by chance, or should I look for it?

Comment 6

[Boris Zbarsky [:bzbarsky]]

And yes, the new behavior was definitely introduced in bug 522601.

Comment 7

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #5)

> That's the correct behavior of a DOM TreeWalker.  The DOM spec is very clear on
> this issue:

Oops! I've even read that paragraph at least three times without understanding what it actually says.

> So the real issue is that I missed an inDOMTreeWalker consumer that was
> depending on the old, buggy, behavior...
> 
> Colby, did you digging turn up the relevant consumer by chance, or should I
> look for it?

And I'm usually better at cross-referencing bugs, too. Bug 526939.

Comment 8

[Colby Russell :crussell (no longer Mozilla-ing)]

Err... maybe I should pay attention to the bug IDs as well. It's in resources/content/viewers/dom/dom.js FindNext().

Comment 9

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: dom.js cleanup

Comment 10

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: findNext fix that expects inDeepTreeWalker::NextNode's TreeWalker behavior (apply this second)

Comment 11

[Shawn Wilsher :sdwilsh]

Comment on attachment 435502 [details] [diff] [review]
findNext fix that expects inDeepTreeWalker::NextNode's TreeWalker behavior (apply this second)

r=sdwilsh

Comment 12

[Shawn Wilsher :sdwilsh]

Comment on attachment 435501 [details] [diff] [review]
dom.js cleanup

>+    this.setProcessingInstructions(
>+      PrefUtils.getPref("inspector.dom.showProcessingInstructions"));
>+    this.setAccessibleNodes(
>+      PrefUtils.getPref("inspector.dom.showAccessibleNodes"));
>+    this.setWhitespaceNodes(
>+      PrefUtils.getPref("inspector.dom.showWhitespaceNodes"));
nit: for multiline calls like this, do this:
callingFunc(
  arguemnt
);

r=sdwilsh

Comment 13

[Colby Russell :crussell (no longer Mozilla-ing)]

Attachment: dom.js cleanup mkII (apply this first)

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 435502 [details] [diff] [review]
findNext fix that expects inDeepTreeWalker::NextNode's TreeWalker behavior (apply this second)

Oh, awesome.  I'd been planning to look at this code tomorrow, but it looks like I win.

sr=me, and thank you for cleaning up my mess!

Comment 15

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #14)
> Oh, awesome.  I'd been planning to look at this code tomorrow, but it looks
> like I win.
> 
> sr=me, and thank you for cleaning up my mess!

I was impatient. And if I'd realized that that inDeepTreeWalker was doing what it was supposed to be doing, I'd have just done this Friday.

Comment 16

[alexander :surkov (:asurkov)]

landed

http://hg.mozilla.org/dom-inspector/rev/832c66dff1b4
http://hg.mozilla.org/dom-inspector/rev/27b0e9ef1715

I forget to specify -u to point Colby name so checkins appears under my name. If it's important issue then I'd like to fix this. However I need to realize how to do this, please ping me if you know this.

Comment 17

[Boris Zbarsky [:bzbarsky]]

It's somewhat important, but you can't fix it post-fact.  You have to get it right the first time (e.g. by using "hg out" before pushing).

Comment 18

[Shawn Wilsher :sdwilsh]

(In reply to comment #17)
> It's somewhat important, but you can't fix it post-fact.  You have to get it
> right the first time (e.g. by using "hg out" before pushing).
or backout and reland

Comment 19

[alexander :surkov (:asurkov)]

(In reply to comment #18)
> (In reply to comment #17)
> > It's somewhat important, but you can't fix it post-fact.  You have to get it
> > right the first time (e.g. by using "hg out" before pushing).
> or backout and reland

Shawn, please make decision as DOMi owner :)

Comment 20

[Shawn Wilsher :sdwilsh]

(In reply to comment #19)
> Shawn, please make decision as DOMi owner :)
I'll leave it up to the person who wrote the patch.  If they don't care, you can leave it in.

Comment 21

[Colby Russell :crussell (no longer Mozilla-ing)]

I told surkov via that I didn't mind.

Comment 22

[Colby Russell :crussell (no longer Mozilla-ing)]

that's via IRC

Comment 23

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> I told surkov via that I didn't mind.

Ok, thanks. Next time I will be more careful.