450876 - Crash [@ nsEventStateManager::GetNextTabbableMapArea] with img usemap and tabindex

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Martijn Wargers (dead)]

Attachment: testcase

See testcase, which crashes in Firefox 2, Firefox 3 and current trunk build when tabbing in the document.

http://crash-stats.mozilla.com/report/index/adce7268-6b72-11dd-a999-001a4bd43ed6?p=1
0  	xul.dll  	nsEventStateManager::GetNextTabbableMapArea  	 content/events/src/nsEventStateManager.cpp:3955
1 	xul.dll 	nsEventStateManager::GetNextTabbableContent 	content/events/src/nsEventStateManager.cpp:3915

Comment 1

[Martijn Wargers (dead)]

Attachment: patch

Comment 2

[Martijn Wargers (dead)]

Attachment: diff -w of patch

Or rather a hg diff -NprwU20 it seems.

Comment 3

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

perhaps early return?
if (!imageMap) {
  return nsnull;
}

Comment 4

[Martijn Wargers (dead)]

Attachment: patch

I assume the img should not be tabbable, right? That's what IE seems to be doing.

Comment 5

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

(In reply to comment #4) 
> I assume the img should not be tabbable, right? That's what IE seems to be
> doing.
Does that have anything to do with this bug?

Comment 6

[Martijn Wargers (dead)]

Well, the patch makes the img with the non-existing usemap non-tabbable.

Comment 7

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

how? It is just a null check for a case which would otherwise crash when
mapContent is used.

Comment 8

[Martijn Wargers (dead)]

You're right. Would probably better to file a new bug for that (different issue (if issue at all)).

Comment 9

[Martijn Wargers (dead)]

Hrm, with a slightly different testcase, I'm crashing in IE8 too.

Comment 10

[Daniel Veditz [:dveditz]]

This appears to be an unexploitable null deref crash if this patch fixes it, and if that's true it's not a blocker. But we'd approve this simple fix if this works for the branches.

Comment 11

[Johnny Stenback (:jst)]

Not a blocker, but feel free to land this any time the tree is green.

Comment 12

[Jesse Ruderman]

Attachment: testcase 2 (crashes Firefox when loaded)

This testcase is fun in that it manages to trigger the tabbing code without any user interaction.

Comment 13

[Reed Loden [:reed]]

http://hg.mozilla.org/mozilla-central/rev/3a616f9b649e

Comment 14

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

Comment on attachment 334106 [details] [diff] [review]
patch

a191=beltzner

Comment 15

[Reed Loden [:reed]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/f305bb515add

Does this same patch apply on 1.9.0 and 1.8.1?

Comment 16

[Samuel Sidler (old account; do not CC)]

Not sure if this applies to 1.8.0.x as well, but I'll let those drivers determine whether they want it or not (since it's a null deref, maybe not even if it does apply).

Comment 17

[Samuel Sidler (old account; do not CC)]

Martijn: Can you please work up 1.9.0 and 1.8.1 patches? Code freeze is technically tomorrow at 11:59pm...

Comment 18

[Samuel Sidler (old account; do not CC)]

Martijn: Please get a patch worked up for this in the next week or two so we can take it in 1.9.0.8.

Comment 19

[Martijn Wargers (dead)]

Attachment: 1.9.0.x patch

Sorry, electricity went down yesterday, which made it impossible to submit a patch.
Do I need to ask review for these patches, I guess? Should I also add the tests?
I haven't tested this patch yet, I don't worry about the patch causing problems, though, more about the test.

Comment 20

[Martijn Wargers (dead)]

Attachment: 1.8.1.x patch

The 1.8.1 tree doesn't seem to have tests.

Comment 21

[Martijn Wargers (dead)]

Comment on attachment 360481 [details] [diff] [review]
1.8.1.x patch

Sorry, never mind, 1.8.1 is Firefox 2, which isn't supported anymore.

Comment 22

[Samuel Sidler (old account; do not CC)]

Comment on attachment 360481 [details] [diff] [review]
1.8.1.x patch

We still want this patch on 1.8.1 for Thunderbird 2 and for Firefox 2 which distros still ship.

Comment 23

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090209 Shiretoko/3.1b3pre
and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090209 Minefield/3.2a1pre

Comment 24

[Daniel Veditz [:dveditz]]

Comment on attachment 360480 [details] [diff] [review]
1.9.0.x patch

sr=dveditz

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 360481 [details] [diff] [review]
1.8.1.x patch

sr=dveditz

Comment 26

[Daniel Veditz [:dveditz]]

Comment on attachment 360480 [details] [diff] [review]
1.9.0.x patch

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 27

[Daniel Veditz [:dveditz]]

Comment on attachment 360481 [details] [diff] [review]
1.8.1.x patch

Approved for 1.8.1.21, a=dveditz for release-drivers

Comment 28

[Martijn Wargers (dead)]

Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.743; previous revision: 1.742
done
Checking in content/events/test/Makefile.in;
/cvsroot/mozilla/content/events/test/Makefile.in,v  <--  Makefile.in
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/content/events/test/test_bug450876.html,v
done
Checking in content/events/test/test_bug450876.html;
/cvsroot/mozilla/content/events/test/test_bug450876.html,v  <--  test_bug450876.html
initial revision: 1.1
done

Checked into the 1.9.0.x tree.

Comment 29

[Martijn Wargers (dead)]

Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <--  nsEventStateManager.cpp
new revision: 1.595.2.32; previous revision: 1.595.2.31
done

Checked into the 1.8.1.x tree.

Comment 30

[Al Billings [:abillings - ex-MoCo]]

Verified 1.9.0.8 using testcase attached and Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.8pre) Gecko/2009031905 GranParadiso/3.0.8pre (.NET CLR 3.5.30729). Crashes 1.9.0.7 cleanly.