Bug 7899 – FrameTree:uniqueChildName doesn't return unique names

• Component: Frames
• Status: NEW

[Justin Garcia]

The number used for unique frame names is the childCount() of the parent frame:
    char suffix[40];
    snprintf(suffix, sizeof(suffix), "/<!--frame%u-->-->", childCount());
This is wrong.  See the attached test case.

[Justin Garcia]

Created attachment 7224 [details]
testcase

Creates two unnamed iframes, removes the first one and reinserts it.  It gets a generated name when it is reinserted, but that name is the same as the frame that is already in the document.  This name collision prevents the inserted frame from being constructed.

[Geoffrey Garen]

Created attachment 11979 [details]
reduction1

Attaching two unique reductions for triggering this bug.

[Geoffrey Garen]

Created attachment 11980 [details]
reduction2

Attaching two unique reductions for how you can trigger this bug.

[Geoffrey Garen]

It would be trivial to fix this bug by replacing childCount() with a never-decrementing count of the number of frames that have been added to the page. In fact, we could gut the whole uniqueChildName function and just use the count for unique-ing.

Since a page can have a maximum of 200 frames, the count would be (virtually) guaranteed to be unique.

The only problem is this comment: "Create a repeatable name." Why does the name need to be repeatable? What does repeatable mean? Using a global count would give a frame a new name if you removed and then re-added it to the document. So I don't think that would be "repeatable."

On the other hand, this bug demonstrates that frames *need* to acquire new names when you remove and then re-add them to the document.

[Darin Adler]

(In reply to comment #4)
> The only problem is this comment: "Create a repeatable name." Why does the name
> need to be repeatable? What does repeatable mean? Using a global count would
> give a frame a new name if you removed and then re-added it to the document. So
> I don't think that would be "repeatable."

My understanding was that the name of the frame needs to be the same for the same page the next time the page is loaded. Something about back/forward perhaps?

[Brett Wilson (Google)]

*** Bug 14895 has been marked as a duplicate of this bug. ***

[Brett Wilson (Google)]

This bug is causing some crashes for me, and is showing up in the topcrash list for my embedding application. Should it be P1?

The crash is in EventHandler::passWheelEventToWidget (and presumably other input events) when you use the scroll wheel over certain iframes (seems to depend on timing, can can be hard to reproduce) because the widget for the RenderWidget is NULL.

The widget is normally set when the load is committed for the frame, which of course requires that the load be started. The load is started from the redirect timer in the FrameLoader.

What happens is that frame 1 comes in and sets the redirect timer to start the load. Other stuff happens, frames get deleted, etc. to cause the name to be the same (this is where the timing sutff comes in). Frame 2 then gets created and happens to get the same "unique" child name as Frame 1. In FrameLoader::requestFrame, we now get the *old* frame in |frame|, cancel the old (correct) load, and start loading the new frame's URL in it. Frame 2 is never initialized, leading to a crash.

[Brett Wilson (Google)]

Actually, it looks like this crash doesn't happen in Safari since the Mac Event handling code filters out sending messages to anything with a NULL widget. However, this still causes frames to not be loaded, as in the example. There could be other subtle bugs and crashes caused by the uninitialized frame, as well as, of course, the missing content.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Actually, it looks like this crash doesn't happen in Safari since the Mac Event
> handling code filters out sending messages to anything with a NULL widget.
> However, this still causes frames to not be loaded, as in the example. There
> could be other subtle bugs and crashes caused by the uninitialized frame, as
> well as, of course, the missing content.

How is it crashing in your application but not in Safari?  Are you using WebKit in your application?  A (partial) stack trace of the issue would be helpful.