Last Comment Bug 704758 - "ASSERTION: null frame" in PresShell::HandleEvent with focus, navigation
: "ASSERTION: null frame" in PresShell::HandleEvent with focus, navigation
: assertion, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla12
Assigned To: Neil Deakin
: Andrew Overholt [:overholt]
Depends on:
Blocks: 594645
  Show dependency treegraph
Reported: 2011-11-22 23:16 PST by Jesse Ruderman
Modified: 2012-01-09 11:28 PST (History)
8 users (show)
enndeakin: in‑testsuite-
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (327 bytes, text/html)
2011-11-22 23:16 PST, Jesse Ruderman
no flags Details
stack trace (5.99 KB, text/plain)
2011-11-22 23:17 PST, Jesse Ruderman
no flags Details
patch (1.26 KB, patch)
2011-11-23 10:12 PST, Neil Deakin
no flags Details | Diff | Splinter Review
updated patch (1.35 KB, patch)
2011-12-06 07:50 PST, Neil Deakin
bugs: review-
Details | Diff | Splinter Review
another updated patch (1.43 KB, patch)
2011-12-19 10:07 PST, Neil Deakin
bugs: review+
Details | Diff | Splinter Review

Description User image Jesse Ruderman 2011-11-22 23:16:49 PST
Created attachment 576413 [details]

1. Set dom.disable_window_flip to false
2. Load the testcase
3. Click the button.

###!!! ASSERTION: null frame: 'aFrame', file layout/base/nsPresShell.cpp, line 5727
Comment 1 User image Jesse Ruderman 2011-11-22 23:17:24 PST
Created attachment 576414 [details]
stack trace
Comment 2 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-11-23 01:55:45 PST
Is this a regression or did we used to get "null view" assertion?
Comment 3 User image Neil Deakin 2011-11-23 10:10:56 PST
The issue here is that this is an IME event that expects to be retargeted to the focused window (the code near the top of nsPresShell::HandleEvent). The recursive call to HandleEvent is called with a null frame and the old code which used to handle getting the frame from a parent view was moved to the view manager.
Comment 4 User image Neil Deakin 2011-11-23 10:12:41 PST
Created attachment 576531 [details] [diff] [review]
Comment 5 User image Neil Deakin 2011-11-24 11:18:43 PST
Comment on attachment 576531 [details] [diff] [review]

NS_IsEventTargetedAtFocusedWindow only differs from the check done at (this code used to be in the presshell) because there is an extra check for NS_IS_CONTENT_COMMAND_EVENT. Not sure if the extra check for these events are needed.

masayuki, this assertion happens during the NS_QUERY_TEXT_CONTENT event. Can you check this over to make this event is still working properly with the patch, or let me know how to test it?
Comment 6 User image Masayuki Nakano [:masayuki] 2011-11-24 17:48:54 PST
If the event is NS_QUERY_TEXT_CONTENT and the root frame hasn't been in the presShell, you can just return. Then, the query event indicates failed. It's better for widget rather than succeeded on unexpected view. I'll fix it on cocoa widget side. (The caller (IMEInputHandler::ResetIMEWindowLevel()) should retry it later)

If the event is a content command event, maybe, we can call nsEventStateManager::DoContentCommandEvent() or nsEventStateManager::DoContentCommandScrollEvent() directly. I think that I should check it later. Your patch should be safe for content command event even though it failed to return correct value. However, I think that you should just return like query event.

So, I think that you should just return if the event is content command event or query content event.
Comment 7 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-11-30 03:14:12 PST
Comment on attachment 576531 [details] [diff] [review]

if (expr) {

But based on comment from Masayuki, I'm expecting a new patch here.
Comment 8 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-12-05 06:10:20 PST
Neil, will you update the patch?
Comment 9 User image Neil Deakin 2011-12-06 07:50:15 PST
Created attachment 579302 [details] [diff] [review]
updated patch
Comment 10 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2011-12-07 02:37:33 PST
Comment on attachment 579302 [details] [diff] [review]
updated patch

># HG changeset patch
># Parent 8367f06e227bbdb752d56e8f11ca4a4554c10417
>diff --git a/layout/base/nsPresShell.cpp b/layout/base/nsPresShell.cpp
>--- a/layout/base/nsPresShell.cpp
>+++ b/layout/base/nsPresShell.cpp
>@@ -5784,17 +5784,36 @@ PresShell::HandleEvent(nsIFrame        *
>     }
>     if (retargetEventDoc) {
>       nsCOMPtr<nsIPresShell> presShell = retargetEventDoc->GetShell();
>       if (!presShell)
>         return NS_OK;
>       if (presShell != this) {
>-        return presShell->HandleEvent(presShell->GetRootFrame(), aEvent, true, aEventStatus);
>+        nsIFrame* frame = presShell->GetRootFrame();
>+        if (!frame) {
>+          if (aEvent->message == NS_QUERY_TEXT_CONTENT ||
>+              NS_IS_CONTENT_COMMAND_EVENT(aEvent))
>+            return NS_OK;
if (expr) {
Though, looks like the code around isn't consistent.

>+          nsIView* view = presShell->GetViewManager()->GetRootView();
>+          while (view && !view->GetFrame()) {
>+            view = view->GetParent();
>+          }
>+          if (view) {
>+            frame = view->GetFrame();
>+          }
>+        }
>+        if (!frame)
>+          return NS_OK;
>+        return presShell->HandleEvent(frame, aEvent, true, aEventStatus);
So, frame may not be in presShell?
That would be bad. Should you retarget to frame->PresShell()?
Comment 11 User image Neil Deakin 2011-12-19 10:07:41 PST
Created attachment 582868 [details] [diff] [review]
another updated patch
Comment 12 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2011-12-21 16:59:55 PST
(FWIW: I hit this assertion in my linux debug build if I visit file:/// and hold down Ctrl+R to reload over & over.  I tried out the latest patch here, and it appears to fix it - woot!)
Comment 13 User image Bernd 2011-12-27 07:54:51 PST
another way to trigger the assert is to enter 


in the url bar and hit enter.
Comment 15 User image Marco Bonardo [::mak] 2011-12-29 03:21:13 PST
Comment 16 User image Alex Keybl [:akeybl] 2012-01-09 11:20:46 PST
It's unclear that this is a common user scenario. Please re-nominate if that's not the case.
Comment 17 User image Daniel Holbert [:dholbert] (vacation, returning 2/27) 2012-01-09 11:28:32 PST
Neil, do you know if this bug has any effects in opt builds?

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