246012 - [FIXr]M17 Crash when pressing f6 on page with iframe and applied custom CSS style [@ nsEventStateManager::IsIFrameDoc ]

Status: RESOLVED FIXED

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

Description

[Joost Smit]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040608 Firefox/0.8.0+

When using *[src*="?ad"] { display: none !important }  to hide ads in
userContent.css firefox and mozilla suite ( FF 0.9rc and Mozilla Suite 1.7rc3)
crash when an iframe is present containing :
<iframe src="?ad=1"></iframe> 

Reproducible: Always
Steps to Reproduce:
1. Add stylesheet rule to userContent.css
2. load page with iframe
3. press f6

Actual Results:  
The browser crashed.


I've entered a report in the talkback agent (id TB81055W) and made a testcase.
Both mozilla suite en firefox were tested with a clean profile.

Comment 1

[Joost Smit]

Attachment: Testcase

Added testcase with userContent style rule, and html containing the iFrame

Comment 2

[Olivier Cahagne]

Attachment: TBTB81055W data

Comment 3

[Olivier Cahagne]

Attachment: On-line testcase (you still need to add userContent.css in your profile)

Comment 4

[Joost Smit]

Attachment: online testcase 2

In the firefox bug forum mw reduced the testcase even further. Only press F6 to
crash the browser. Seems like the problem is display: none; in combination with
a src="?a" 
it won't crash with only src="?" or only src="a", however it will crash when
replacing the a with some more random characters.

Comment 5

[Olivier Cahagne]

also crashing on Linux using FF trunk 20040604: TB82028.

Comment 6

[Joost Smit]

(In reply to comment #4)
> Created an attachment (id=150348)
> online testcase 2

Hmm..this is weird. The local html file i used for this testcase crashes when
pressing F6, but the version here on bugzilla won't crash. The code is exactly
the same.

Comment 7

[Rene Pronk]

Sounds like a dupe of bug 146255 (same callstack) but that one is supposed to be
fixed.

Comment 8

[Martijn Wargers (dead)]

The online testcase 2 does not crash on me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/2004-06-01
Firefox/0.8.0+
But it does crash, using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8a2) Gecko/2004-06-02
Firefox/0.8.0+

Comment 9

[Joost Smit]

online testcase 2 crashes here when i save it to a local file with firefox 0.9rc
and mozilla suite 1.7rc3 (Without  the css rule in userContent.css) 
It doesn't crash in both browser when i use the online testcase online here. 

However, when i add the css rule in userContent.css it crashes both browsers
(online and local)

Comment 10

[Boris Zbarsky [:bzbarsky]]

This is probably the same issue as bug 146255, just with a different docshell...

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix it

I can't actually reproduce this crash in my devel build; can someone seeing the
crash test the patch?

Comment 12

[Johnny Stenback (:jst)]

Comment on attachment 150365 [details] [diff] [review]
This should fix it

 PRBool
 nsEventStateManager::IsIFrameDoc(nsIDocShell* aDocShell)
 {
+  // XXXbz wouldn't it be better to use our window's owning content
+  // and not deal with all this mess?

Um, yes, it would...

+  // The parent docshell may not have a presshell, so just get the
+  // document directly.
+  nsCOMPtr<nsIDocument> parentDoc = do_GetInterface(parentItem);
+  if (!parentDoc) {
+    NS_ERROR("We're a child of a docshell without a document?");
+    return PR_FALSE;
+  }

Unless I missed something, do_GetInterface() to an nsIDocument on a docshell
will always return null. nsIDOMDocument would work, but not nsIDocument.

r+sr=jst with the above fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not going to get a chance to really build-and-test this until July 11 at
this point... if someone can start with the attached patch and adjust it per
jst's review, that would be great.

Comment 14

[Martijn Wargers (dead)]

(In reply to comment #11)
> I can't actually reproduce this crash in my devel build; can someone seeing the
> crash test the patch?
I've applied the patch and rebuild my firefox build.
It doesn't crash anymore with the patch applied.
Instead, I get an ASSERTION: We're a child of a docshell without a document?:
'Error', file C:/mozilla/mozilla/content/events/src/nsEventStateManager.cpp,
line 5204

F6 doesn't seem to select the url bar anymore with the patched build in this
testcase.

By the way, the testcase does seem to crash also the 1.7 branch. At least it
crashes with me, using:
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/2004-06-08
Firefox/0.8.0+

Comment 15

[Jay Patel [:jay]]

Adding topcrash keyword since I see a few of these crashes in early M17 Talkback
data and it's easily reproducible.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to comments

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153244 [details] [diff] [review]
Patch updated to comments

jst, how's this?

Comment 18

[Johnny Stenback (:jst)]

Comment on attachment 153244 [details] [diff] [review]
Patch updated to comments

r+sr=jst

Comment 19

[Boris Zbarsky [:bzbarsky]]

Taking.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Checked in for 1.8a3 or 1.8b or whatever this next milestone is.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 153244 [details] [diff] [review]
Patch updated to comments

I think this is worth taking on the 1.7 branch.

Comment 22

[Mike Kaply [:mkaply]]

Comment on attachment 153244 [details] [diff] [review]
Patch updated to comments

a=mkaply - what about aviary?

Comment 23

[Boris Zbarsky [:bzbarsky]]

Fixed on 1.7 branch.

Comment 24

[Michael Lefevre]

Anything with 1.7 branch approval has automatic approval for aviary, doesn't 
it?  I guess someone will sync up aviary with the 1.7.2 changes at some point 
anyway...

Comment 25

[Olivier Cahagne]

Given bug 254539 affecting Firefox and maybe dupe of this one, nominating for
Aviary 1.0PR (in addition to the whiteboard, sorry if it's redundant) to make
sure it's in the scope when quering all blocking-aviary bugs.

Comment 26

[Olivier Cahagne]

*** Bug 254539 has been marked as a duplicate of this bug. ***

Comment 27

[Ryan Polk (Quark)]

*** Bug 255254 has been marked as a duplicate of this bug. ***