Last Comment Bug 450876 - Crash [@ nsEventStateManager::GetNextTabbableMapArea] with img usemap and tabindex
: Crash [@ nsEventStateManager::GetNextTabbableMapArea] with img usemap and tab...
Status: VERIFIED FIXED
[sg:nse] null deref
: crash, fixed1.8.1.21, testcase, verified1.9.0.9, verified1.9.1
Product: Core
Classification: Components
Component: Event Handling (show other bugs)
: Trunk
: x86 Windows XP
: -- critical (vote)
: mozilla1.9.2a1
Assigned To: Martijn Wargers [:mwargers] (not working for Mozilla)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-08-16 07:18 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-06-13 10:01 PDT (History)
8 users (show)
jst: blocking1.9.1-
samuel.sidler+old: blocking1.9.0.9+
samuel.sidler+old: wanted1.9.0.x+
samuel.sidler+old: wanted1.8.1.x+
samuel.sidler+old: blocking1.8.0.next?
samuel.sidler+old: wanted1.8.0.x?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (182 bytes, text/html)
2008-08-16 07:18 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
patch (4.88 KB, patch)
2008-08-16 09:55 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
diff -w of patch (2.53 KB, patch)
2008-08-16 10:11 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
patch (3.26 KB, patch)
2008-08-16 11:38 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
bugs: review+
roc: superreview+
mbeltzner: approval1.9.1+
Details | Diff | Splinter Review
testcase 2 (crashes Firefox when loaded) (206 bytes, application/xhtml+xml)
2008-12-02 14:01 PST, Jesse Ruderman
no flags Details
1.9.0.x patch (3.80 KB, patch)
2009-02-04 03:16 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
dveditz: superreview+
dveditz: approval1.9.0.9+
Details | Diff | Splinter Review
1.8.1.x patch (1.29 KB, patch)
2009-02-04 03:21 PST, Martijn Wargers [:mwargers] (not working for Mozilla)
dveditz: superreview+
dveditz: approval1.8.1.next+
dveditz: approval1.8.0.next?
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-16 07:18:52 PDT
Created attachment 334091 [details]
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 [:mwargers] (not working for Mozilla) 2008-08-16 09:55:35 PDT
Created attachment 334103 [details] [diff] [review]
patch
Comment 2 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-16 10:11:18 PDT
Created attachment 334104 [details] [diff] [review]
diff -w of patch

Or rather a hg diff -NprwU20 it seems.
Comment 3 Olli Pettay [:smaug] 2008-08-16 10:16:55 PDT
perhaps early return?
if (!imageMap) {
  return nsnull;
}
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-16 11:38:36 PDT
Created attachment 334106 [details] [diff] [review]
patch

I assume the img should not be tabbable, right? That's what IE seems to be doing.
Comment 5 Olli Pettay [:smaug] 2008-08-16 11:41:28 PDT
(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 [:mwargers] (not working for Mozilla) 2008-08-16 11:45:01 PDT
Well, the patch makes the img with the non-existing usemap non-tabbable.
Comment 7 Olli Pettay [:smaug] 2008-08-16 11:49:26 PDT
how? It is just a null check for a case which would otherwise crash when
mapContent is used.
Comment 8 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-16 12:02:58 PDT
You're right. Would probably better to file a new bug for that (different issue (if issue at all)).
Comment 9 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-16 12:15:07 PDT
Hrm, with a slightly different testcase, I'm crashing in IE8 too.
Comment 10 Daniel Veditz [:dveditz] 2008-08-18 11:46:05 PDT
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, jst@mozilla.com) 2008-09-24 15:41:35 PDT
Not a blocker, but feel free to land this any time the tree is green.
Comment 12 Jesse Ruderman 2008-12-02 14:01:40 PST
Created attachment 351051 [details]
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] (use needinfo?) 2008-12-14 22:28:15 PST
http://hg.mozilla.org/mozilla-central/rev/3a616f9b649e
Comment 14 Mike Beltzner [:beltzner, not reading bugmail] 2008-12-29 09:04:13 PST
Comment on attachment 334106 [details] [diff] [review]
patch

a191=beltzner
Comment 15 Reed Loden [:reed] (use needinfo?) 2009-01-01 23:22:25 PST
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) 2009-01-03 02:46:53 PST
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) 2009-02-02 07:44:50 PST
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) 2009-02-03 21:11:25 PST
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 [:mwargers] (not working for Mozilla) 2009-02-04 03:16:49 PST
Created attachment 360480 [details] [diff] [review]
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 [:mwargers] (not working for Mozilla) 2009-02-04 03:21:14 PST
Created attachment 360481 [details] [diff] [review]
1.8.1.x patch

The 1.8.1 tree doesn't seem to have tests.
Comment 21 Martijn Wargers [:mwargers] (not working for Mozilla) 2009-02-04 03:23:08 PST
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) 2009-02-04 11:08:40 PST
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] 2009-02-09 18:04:09 PST
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] 2009-02-13 07:57:54 PST
Comment on attachment 360480 [details] [diff] [review]
1.9.0.x patch

sr=dveditz
Comment 25 Daniel Veditz [:dveditz] 2009-02-13 07:58:10 PST
Comment on attachment 360481 [details] [diff] [review]
1.8.1.x patch

sr=dveditz
Comment 26 Daniel Veditz [:dveditz] 2009-02-20 11:42:58 PST
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] 2009-02-20 11:43:21 PST
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 [:mwargers] (not working for Mozilla) 2009-02-24 07:20:51 PST
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 [:mwargers] (not working for Mozilla) 2009-02-24 07:24:48 PST
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] 2009-03-19 14:48:34 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.