Closed Bug 118685 Opened 24 years ago Closed 22 years ago

browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent]

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
All
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aleph1, Assigned: MatsPalmgren_bugz)

References

Details

(Keywords: crash, fixed1.4.2, testcase)

Crash Data

Attachments

(6 files, 5 obsolete files)

testcase (code from reporter comment)
140 bytes, text/html
Details
Some crash data from GDB (Linux)
3.21 KB, text/plain
Details
Patch rev. 5
6.09 KB, patch
dbaron
: superreview+
mkaply
: approval1.4.2+
Details | Diff | Splinter Review
Testcase #2 (reduced from bug 207604)
410 bytes, text/html
Details
Patch2 rev. 2
602 bytes, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
Patch rev. 6 (for 1.4 branch)
5.34 KB, patch
bzbarsky
: review+
mkaply
: approval1.0.x+
Details | Diff | Splinter Review
This appears to be a combination of at least two bugs. First, form input type of "file" (file selector) is not collapsed when its "visibility" is set to "collapse" via CSS. Instead it appears to be only "hidden". If the form file input happens to also have the attribute of "disabled" and the user clicks on the area of the page the input is in the browser will crash. Use the follwoing page to test (click inside the box): <html> <body> <form> <table border=1><tr><td> <input type="file" disabled style="visibility: collapse"> </td></tr></table> </form> </html>
Severity: normal → critical
Keywords: crash
Confirming on Linux 2001010621. Reporter, please always include the Build ID in your bug reports. Thank's for testing mozilla. Talkback ID TB1371531K
Hmpf, my build ID is 2002010621 not 2001010621.
A small correction. The browser is actually rendering the input file element with visibility of "collapse" correctly. Acording to the CCS2 spec ( http://www.w3.org/TR/REC-CSS2/visufx.html#visibility ) "if used on elements other than rows or columns, 'collapse' has the same meaning as 'hidden'." Yet, when you use 'collapse' on other input elements (e.g. <input type="password">) the elements does collapse instead of only being hidden. Thus, the bug display bug (not the crach one) appears to be the inverse of what I thought.
Testcase is still crashing Linux 2002011108. New TB ID TB1622970E.
Stack Signature nsEventStateManager::PreHandleEvent() 52cdb48b Trigger Time 2002-01-19 00:00:04 Email Address caillon@returnzero.com URL visited http://bugzilla.mozilla.org/attachment.cgi?id=64787&action=view User Comments Bug 118685 Build ID 2002011821 Product ID MozillaTrunk Platform Operating System LinuxIntel Module Trigger Reason SIGSEGV: Segmentation Fault: (signal 11) Stack Trace nsEventStateManager::PreHandleEvent() PresShell::HandleEventInternal() PresShell::HandleEvent() nsView::HandleEvent() nsView::HandleEvent() nsView::HandleEvent() nsViewManager::DispatchEvent() HandleEvent() nsWidget::DispatchEvent() nsWidget::DispatchWindowEvent() nsWidget::DispatchMouseEvent() nsWidget::OnMotionNotifySignal() nsWindow::HandleGDKEvent() dispatch_superwin_event() handle_gdk_event() libgdk-1.2.so.0 + 0x17de4 (0x40359de4) libglib-1.2.so.0 + 0x10c26 (0x4038bc26) libglib-1.2.so.0 + 0x11253 (0x4038c253) libglib-1.2.so.0 + 0x1141c (0x4038c41c) libgtk-1.2.so.0 + 0x9285c (0x402a485c) nsAppShell::Run() nsAppShellService::Run() main1() main() libc.so.6 + 0x1d7ee (0x404d97ee)
Trying event handling. See also bug 93471 and bug 101834 for similar crashes.
Assignee: asa → joki
Status: UNCONFIRMED → NEW
Component: Browser-General → Event Handling
Ever confirmed: true
QA Contact: doronr → madhur
Christopher, yea, those indicate similair stacks, likely dupes. leaving it up to joki for dup'ing if he deems necessary
Target Milestone: --- → mozilla1.0
Target Milestone: mozilla1.0 → mozilla1.1
QA Contact: madhur → rakeshmishra
QA Contact: rakeshmishra → trix
WFM Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4b) Gecko/20030523 Mozilla Firebird/0.6
See also bug 207604 for a similar crash.
Target Milestone: mozilla1.1alpha → ---
giving to bz
Assignee: joki → bzbarsky
Blocks: 93471, 101834, 207604, 212338
Summary: browser crashes if user clicks on a disabled and collapsed file selection form input → browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent]
.
Assignee: bzbarsky → saari
QA Contact: trix → desale
Attached patch Patch rev. 1 (obsolete) — — Splinter Review
The patch fixes this bug and bug 212338 (and all its dupes), it does not fix bug 207604 (probably a similar issue but it can be handled separately IMO.)
Attachment #132645 - Flags: superreview?(bryner)
Attachment #132645 - Flags: review?(john)
Comment on attachment 132645 [details] [diff] [review] Patch rev. 1 >Index: content/events/src/nsEventStateManager.cpp >=================================================================== >RCS file: /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v >retrieving revision 1.456 >diff -u -r1.456 nsEventStateManager.cpp >--- content/events/src/nsEventStateManager.cpp 27 Sep 2003 04:18:01 -0000 1.456 >+++ content/events/src/nsEventStateManager.cpp 4 Oct 2003 20:58:25 -0000 >@@ -3214,8 +3211,17 @@ > > nsCOMPtr<nsIContent> oldFocus(mCurrentFocus); > ChangeFocus(nextFocus, eEventFocusedByKey); >- mCurrentFocusFrame = nextFocusFrame; >- SetFrameExternalReference(mCurrentFocusFrame); >+ if (!mCurrentFocus && oldFocus) { >+ // ChangeFocus failed to move focus to nextFocus because a blur handler >+ // made it unfocusable. (bug #118685) >+ mCurrentTarget = oldTarget; >+ SetFrameExternalReference(mCurrentTarget); The old mCurrentTarget should still have the external reference bit set on it from before, shouldn't it? Also, it seems like resetting mCurrentTarget might also be a good idea a few lines down (the doc != nextFocus->GetDocument() case). >+ } else { >+ GetFocusedFrame(&nextFocusFrame); >+ if (nextFocusFrame) >+ SetFrameExternalReference(nextFocusFrame); >+ } > > // It's possible that the act of removing focus from our previously > // focused element caused nextFocus to be removed from the document. >@@ -4113,7 +4115,7 @@ > nsCOMPtr<nsIPresShell> presShell; > aPresContext->GetShell(getter_AddRefs(presShell)); > >- nsCOMPtr<nsIContent> previousFocus = mCurrentFocus; >+ nsIContent* previousFocus = mCurrentFocus; > Is it possible for the element to be destroyed while dispatching a blur to it, so that previousFocus becomes a dangling reference? My best guess at how you could cause that to happen is make the blur handler focus another element which then destroys the original element. It's probably safer to use a strong reference, unless I'm missing something that ensures the element can't be destroyed from under us. >@@ -4124,7 +4126,7 @@ > > if (nsnull != gLastFocusedPresContext) { > >- nsCOMPtr<nsIContent> focusAfterBlur; >+ nsIContent* focusAfterBlur = nsnull; > This, however, should be fine, since there's no DOM event dispatch between setting the value and checking it. >@@ -4269,11 +4271,20 @@ > } > } > >+ { >+ // Check if the HandleDOMEvent calls above destroyed our frame (bug #118685) >+ nsIFrame* frame = nsnull; >+ if (aContent) >+ presShell->GetPrimaryFrameFor(aContent, &frame); >+ if (!frame) { >+ aContent = nsnull; >+ } >+ } >+ This is definitely good - we don't want to focus frameless content, ever. All of the cleanup to use SetFocusedContent() look good as well.
Comment on attachment 132645 [details] [diff] [review] Patch rev. 1 marking sr- to try to get review comments addressed :)
Attachment #132645 - Flags: superreview?(bryner) → superreview-
>+ mCurrentTarget = oldTarget; >+ SetFrameExternalReference(mCurrentTarget); # The old mCurrentTarget should still have the external reference bit set on it # from before, shouldn't it? Indeed. Also, with blur/focus handlers that destroy content in mind, I don't think we can trust this frame to be alive at this point. I changed the code to |mCurrentTarget = nsnull;| >- nsCOMPtr<nsIContent> previousFocus = mCurrentFocus; >+ nsIContent* previousFocus = mCurrentFocus; > # Is it possible for the element to be destroyed while dispatching a blur to it, # so that previousFocus becomes a dangling reference? Yes. # It's probably safer to use a strong # reference, unless I'm missing something that ensures the element can't be # destroyed from under us. This isn't really needed since we don't dereference the pointers. Both |previousFocus| and |focusAfterBlur| are only used to compare *pointer values* after the handlers have been called. I have tested many cases with nasty event handlers that destroys content and frames without problems.
Assignee: saari → mats.palmgren
Summary: browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent] → [FIX] browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent]
Attached patch Patch rev. 2 (for review only) (obsolete) — — Splinter Review
Changes since rev. 1: update |mCurrentTarget| differently. NOTE: this patch is not against HEAD and is for review only. (I'm going to update my tree now to get the "deCOMtaminate nsIDocument" changes - expect an updated patch shortly)
Attachment #132645 - Attachment is obsolete: true
Attachment #132645 - Flags: review?(john)
Attachment #133902 - Flags: superreview?(dbaron)
Attachment #133902 - Flags: review?(bryner)
Status: NEW → ASSIGNED
Flags: blocking1.6a?
Attached patch Patch rev. 3 (obsolete) — — Splinter Review
This patch is against HEAD (1.459) as promised - there is no difference compared with rev. 2 except for the line numbers so it's probably not needed. I reviewed and retested in a current tree anyway and everything looked fine.
crash on testcase: TalkbackID TB24790292X Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031024 Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.6a) Gecko/20031025 changing OS to all, as this crashes on windows also. Talkback from BuildID 2003102404 as I couldn´t get Talkback from BuildID 2003102504 working.
OS: Linux → All
Time is short for 1.6a. If this is gonna make it, then it's going to need some quick reviews.
Flags: blocking1.6a? → blocking1.6a-
Comment on attachment 133902 [details] [diff] [review] Patch rev. 2 (for review only) I'm going to be paranoid and note that a pointer value comparison isn't guaranteed to be valid after the object is destroyed -- because the memory could, in theory, have been reallocated to a different object that is now mCurrentFocus. Very unlikely, sure, but why take the chance? This isn't performance critical code.
Comment on attachment 133902 [details] [diff] [review] Patch rev. 2 (for review only) r=bryner with the above comment addressed.
Attachment #133902 - Flags: review?(bryner) → review+
*** Bug 224070 has been marked as a duplicate of this bug. ***
Attached patch Patch rev. 4 (obsolete) — — Splinter Review
Good point. I have reverted that bit.
Attachment #133902 - Attachment is obsolete: true
Attachment #133956 - Attachment is obsolete: true
Attachment #133902 - Flags: superreview?(dbaron)
Attachment #134428 - Flags: superreview?(dbaron)
Comment on attachment 134428 [details] [diff] [review] Patch rev. 4 >+ { >+ // Check if the HandleDOMEvent calls above destroyed our frame (bug #118685) >+ nsIFrame* frame = nsnull; >+ if (aContent) >+ presShell->GetPrimaryFrameFor(aContent, &frame); >+ if (!frame) { >+ aContent = nsnull; >+ } >+ } Drive by nit: Aside from your inconsistent bracing style here for a one-line-if, you could slightly re-work the above code since there is no reason to bother with declaring frame or setting aContent to null if aContent already is null. if (aContent) { nsIFrame* frame = nsnull; presShell->GetPrimaryFrameFor(aContent, &frame); if (!frame) { aContent = nsnull; } }
Attached patch Patch rev. 5 — — Splinter Review
Fixed as suggested.
Attachment #134428 - Attachment is obsolete: true
Attachment #134428 - Flags: superreview?(dbaron)
Attachment #134431 - Flags: superreview?(dbaron)
Attachment #134431 - Flags: superreview?(dbaron) → superreview+
This crash also happens on the 1.4 branch. Is there a way we could get a patch for the 1.4 branch also? Thanks
*** Bug 224875 has been marked as a duplicate of this bug. ***
Can someone check this in for me please. I have r=bryner in comment 22 on rev.3 where he rubberstamps the change in rev.4, the change in rev.5 is purely cosmetical and has sr=dbaron.
Checking in content/events/src/nsEventStateManager.cpp; /cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v <-- nsEventStateManager.cpp new revision: 1.462; previous revision: 1.461 done Mats, you should really just get a CVS account... ;)
I have verified that bug 212338, bug 212988, bug 213397, bug 214471, bug 221109 and bug 224875 is now fixed. (build 2003-11-08-05) We still crash on the testcase attached to this bug, but I can't reproduce that in my debug build :-( bug 207604 and bug 224070 also still crash so I'm leaving the bug open...
Summary: [FIX] browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent] → browser crashes if user clicks on a disabled and collapsed file selection form input [@ nsEventStateManager::PreHandleEvent]
So... I see the crash in a non-debug optimized build with symbols, but it's hard to figure out what's going on because of the inlining. When I build non-debug non-optimized with symbols I do NOT see the crash... I'll try a lower optimization level and see whether that reproduces the crash, I guess...
I can reproduce this one in my debug build. I'm pretty sure all three crashers are the same problem. They all involve a <input disabled> inside a visibility:hidden block. We crash in PresShell::HandleEvent() because we get a random value back from nsFileControlFrame::GetFrameForPoint() Patch coming up...
Attached patch Patch2 rev. 1 (obsolete) — — Splinter Review
This fixes the bug I think. Could you try that in an opt build? Note: this is not the final patch - I want to nail down who passes in a random value first...
Yeah, that fixes the crash. The callee is not really at fault -- it really should be OK to pass an un-inited value to a method like this.
Comment on attachment 135076 [details] [diff] [review] Patch2 rev. 1 OK, this is the final patch then. I have also looked through all GetFrameForPoint() implementations to make sure they all set a value when returning NS_OK.
Attachment #135076 - Attachment description: wip → Patch2 rev. 1
Attachment #135076 - Attachment filename: patch-2-wip.diff → patch-2-r1.diff
Attachment #135076 - Flags: superreview?(bryner)
Attachment #135076 - Flags: review?(bz-vacation)
Comment on attachment 135076 [details] [diff] [review] Patch2 rev. 1 Actaully, I would prefer that this case return NS_ERROR_FAILURE and not set aFrame (making it more like the nsFrame::GetFrameForPoint code).
Attached patch Patch2 rev. 2 — — Splinter Review
OK, did that.
Attachment #135076 - Attachment is obsolete: true
Attachment #135076 - Flags: superreview?(bryner)
Attachment #135076 - Flags: review?(bz-vacation)
Attachment #135083 - Flags: superreview?(bryner)
Attachment #135083 - Flags: review?(bz-vacation)
Comment on attachment 135083 [details] [diff] [review] Patch2 rev. 2 r+sr=bzbarsky
Attachment #135083 - Flags: superreview?(bryner)
Attachment #135083 - Flags: superreview+
Attachment #135083 - Flags: review?(bz-vacation)
Attachment #135083 - Flags: review+
Checking in layout/html/forms/src/nsFileControlFrame.cpp; /cvsroot/mozilla/layout/html/forms/src/nsFileControlFrame.cpp,v <-- nsFileControlFrame.cpp new revision: 3.149; previous revision: 3.148 done
2003-11-09-05 trunk Linux: this bug, bug 207604 and bug 224070 now works. -> FIXED
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Keywords: testcase
Resolution: --- → FIXED
No longer blocks: 207604
*** Bug 207604 has been marked as a duplicate of this bug. ***
Flagging for 1.4.2, since it was asked for in comment 27
Flags: blocking1.4.2?
Attachment #135083 - Flags: approval1.4.2?
which patch needs to go into 1.4.2? Just the last one?
Both of them are needed also on the 1.4 branch. "Patch rev. 5" fixes bug 212338, bug 212988, bug 213397, bug 214471, bug 221109 and bug 224875 "Patch2 rev. 2" fixes this bug, bug 207604 and bug 224070.
Please mark all appropriate patches that need approval. Is anyone aware of any regressions as a result of this patch?
Attachment #134431 - Flags: approval1.4.2?
Comment on attachment 134431 [details] [diff] [review] Patch rev. 5 a=mkaply get it in quick. please use fixed1.4.2 keyword
Attachment #134431 - Flags: approval1.4.2? → approval1.4.2+
I've checked in "patch 2 rev 2" on the 1.4 branch. "Patch rev 5" does not apply to the branch; it may need to be modified a bit to be landed there.....
wasn't patch rev 5 the big part of this patch?
Yes, it was. The two patches are unrelated, though, so landing one without the other is ok.
Applying "Patch rev. 5" to the 1.4 branch gave the result: patching file content/events/src/nsEventStateManager.cpp Hunk #1 succeeded at 531 (offset 6 lines). Hunk #2 succeeded at 544 (offset 6 lines). Hunk #3 succeeded at 3104 (offset 70 lines). Hunk #4 succeeded at 3126 (offset 71 lines). Hunk #5 succeeded at 3177 (offset 71 lines). Hunk #6 FAILED at 3247. Hunk #7 succeeded at 3303 (offset 77 lines). Hunk #8 succeeded at 3317 (offset 77 lines). Hunk #9 succeeded at 3379 (offset 80 lines). Hunk #10 succeeded at 4315 (offset 94 lines). Hunk #11 succeeded at 4441 (offset 97 lines). Hunk #12 FAILED at 4811. Hunk #13 FAILED at 4820. For Hunk 6 I reCOMified the following: if (oldFocus && doc != nextFocus->GetDocument()) { mCurrentTarget = nsnull; return ShiftFocusInternal(aForward, oldFocus); } to this for the 1.4 branch: if (oldFocus) { nsCOMPtr<nsIDocument> newElementDocument; nextFocus->GetDocument(*getter_AddRefs(newElementDocument)); if (doc != newElementDocument) { mCurrentTarget = nsnull; return ShiftFocusInternal(aForward, oldFocus); } } Hunk 12 and 13 were both in nsEventStateManager::FocusElementButNotDocument and are not needed. There were no other problematic assignments to mCurrentFocus/mCurrentFocusFrame in the 1.4 branch that needed to be fixed. I have verified that all the intended crashes are fixed by this patch: bug 212338, bug 212988, bug 213397, bug 214471, bug 221109 and bug 224875.
Attachment #139798 - Flags: review?(bz-vacation)
Attachment #139798 - Flags: approval1.4.2?
Comment on attachment 139798 [details] [diff] [review] Patch rev. 6 (for 1.4 branch) r=bzbarsky This is basically like the other patch, indeed.
Attachment #139798 - Flags: review?(bz-vacation) → review+
I checked in that patch on the branch.
Keywords: fixed1.4.2
Attachment #139798 - Flags: approval1.0.x+
"Patch rev. 5" caused a regression, bug 232368, when combined with bug 147927. The regression does not occur on the 1.4 branch (probably since bug 147927 was not checked in there.)
Flags: blocking1.4.2?
Attachment #135083 - Flags: approval1.4.2?
Attachment #139798 - Flags: approval1.4.2?
Crash Signature: [@ nsEventStateManager::PreHandleEvent]
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: