Bug 12251 – REGRESSION (r18822-r18823): Assertion failure opening document with non-existent resources (dom/xhtml/level2/html/HTMLIFrameElement11.xhtml)

• Component: Page Loading
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

In my locally-build debug build of WebKit r18824, dom/xhtml/level2/html/HTMLIFrameElement11.xhtml fails with an assertion error:

ASSERTION FAILED: _private->identifierMap->contains(identifier)
(/path/to/WebKit/WebKit/WebView/WebView.mm:3696 -[WebView(WebViewInternal) _objectForIdentifier:])
Segmentation fault

[David Kilzer (:ddkilzer)]

Created attachment 12413 [details]
Stack trace

[David Kilzer (:ddkilzer)]

Confirmed that this broke with r18822-18823.

[Adam Roben (:aroben)]

fast/frames/empty-frame-src.html is failing with the same assertion.

[Adam Roben (:aroben)]

fast/frames/frame-element-name.html also suffers

[Adam Roben (:aroben)]

And fast/frames/frame-js-url-clientWidth.html

[Adam Roben (:aroben)]

And fast/frames/frame-limit.html, fast/frames/frame-name-reset.html, fast/frames/frame-set-same-location.html, fast/frames/frame-set-same-src.html, fast/frames/frame-src-attribute.html, fast/frames/frameElement-frame.html, and probably many others.

[David Kilzer (:ddkilzer)]

(In reply to comment #0)
> In my locally-build debug build of WebKit r18824,
> dom/xhtml/level2/html/HTMLIFrameElement11.xhtml fails with an assertion error:

The problem here is that HTMLIFrameElement11.xhtml references two files that don't exist via <iframe> tags.  The assertion code added in r18822 trips because these files (resources) aren't found and thus aren't loaded.  The solution is to add equivalent nil checks.

What's more disconcerting is that both frame.xhtml and iframe.xhtml exist in the same directory as HTMLIFrameElement11.xhtml, but apparently that's what the actual test does as well:

http://dev.w3.org/cvsweb/2001/DOM-Test-Suite/tests/level2/html/files/iframe2.html?rev=1.4

I have a patch to fix this.

[David Kilzer (:ddkilzer)]

Created attachment 12424 [details]
Patch v1

This is the obvious fix--to do the equivalent nil checking when retrieving an object or removing it from the hash map.

Not sure if the callers of these methods should be smarter about when they call them, e.g., after an attempt was made to load a resource that didn't exist.

Running layout tests now; will report results when completed.

(In reply to comment #7)

> The problem here is that HTMLIFrameElement11.xhtml references two files that
> don't exist via <iframe> tags.  The assertion code added in r18822 trips
> because these files (resources) aren't found and thus aren't loaded.

This isn't quite accurate. Loader items for the missing resources are created. The assertion trips because when the resource fails to load, FrameLoader::didFailToLoad() is invoked twice, once from ResourceLoader::didFail(), then another time from ResourceLoader::didCancel(). In the first time, the identifier is removed from the map. Thus in the second time, it's missing and the assretion fails.

[Darin Adler]

Comment on attachment 12424 [details]
Patch v1

We definitely need to fix this! Can't have layout tests failing.

But HashMap remove already has the behavior of removing it only if it's there, so no need for the contains call.

And if the object that comes back from _objectForIdentifier is nil, then the caller needs to not send the notification.

I think we should land a fixed-up version of this now. The real problem is that the loader is sending multiple "done loading" calls to the client for the same resource -- it can be fixed entirely on the WebCore side. But it's a complex job that we should do later. This approach is correct for now.

[David Kilzer (:ddkilzer)]

(In reply to comment #8)
> Running layout tests now; will report results when completed.

No new regressions with this change, but I had to call run-webkit-tests with these arguments to get the non-patched version to finish:

./Tools/Scripts/run-webkit-tests --ignore-tests dom/xhtml/level2/html/HTMLIFrameElement11.xhtml,fast/frames

[David Kilzer (:ddkilzer)]

Created attachment 12428 [details]
Patch v2

Updated per Comment #10.

Reran failing tests (dom/xhtml/level2/html/HTMLIFrameElement11.xhtml, fast/frames) and they still pass.

[Darin Adler]

Comment on attachment 12428 [details]
Patch v2

Looks fine for a first cut. We need to follow through and make sure that we don't pass the nil object through to the delegate callbacks. But I think that can wait.

r=me

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> Looks fine for a first cut. We need to follow through and make sure that we
> don't pass the nil object through to the delegate callbacks. But I think that
> can wait.

Anders mentioned that he's working on it in IRC today.

Committed revision 18846.