203345 - Imagemaps need to be updated to properly check for XHTML docs

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Boris Zbarsky [:bzbarsky]]

The code at
http://lxr.mozilla.org/seamonkey/source/content/shared/src/nsImageMapUtils.cpp#87
seems to be semi-bogus now that bug 111514 has been fixed.

Comment 1

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 121705 [details] [diff] [review]
Proposed fix.

Excellent.  ;)

Comment 3

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Comment on attachment 121705 [details] [diff] [review]
Proposed fix.

>Index: content/html/document/src/nsHTMLDocument.cpp
>===================================================================
>-    if (map && NS_SUCCEEDED(map->GetName(name))) {
>-      PRBool match;
>+    NS_ASSERTION(map, "Null map in map list!");

We used to return NS_ERROR_FAILURE, with this change we would crash. Please
make it so we return the error and won't crash if !map, and r=heikki.

Comment 4

[Johnny Stenback (:jst)]

We won't hit the !map assertion unless someone breaks some code... mImageMaps is
only filled with non-null nsIDOMHTMLMapElement pointers, strongly typed at
compile time. There's no need to error check that, IMO.

Let me know if you strongly disagree.

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I can't say I strongly disagree, but if I can make something crash-proof I'd
rather do it.

Comment 6

[Johnny Stenback (:jst)]

Fix checked in.

Comment 7

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I'd like to get this on the 1.4 branch as well.

Comment 8

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Reopening to get this evaluated for the branch.

Comment 9

[Asa Dotzler [:asa]]

Comment on attachment 121705 [details] [diff] [review]
Proposed fix.

a=asa (on behalf of drivers) for checkin to the 1.4 branch.

Comment 10

[Samir Gehani]

adt: nsbeta1-

Comment 11

[Samir Gehani]

a=adt to land this on the 1.4 branch.  Please add the fixed1.4 keyword once this
lands.

Comment 12

[Johnny Stenback (:jst)]

Fixed on the branch now too.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Verified, trunk and branch.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: testcase