Last Comment Bug 704758 - "ASSERTION: null frame" in PresShell::HandleEvent with focus, navigation
: "ASSERTION: null frame" in PresShell::HandleEvent with focus, navigation
Status: RESOLVED FIXED
: assertion, testcase
Product: Core
Classification: Components
Component: DOM: Events (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Neil Deakin
:
Mentors:
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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
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 Jesse Ruderman 2011-11-22 23:16:49 PST
Created attachment 576413 [details]
testcase

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 Jesse Ruderman 2011-11-22 23:17:24 PST
Created attachment 576414 [details]
stack trace
Comment 2 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-23 01:55:45 PST
Is this a regression or did we used to get "null view" assertion?
Comment 3 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 Neil Deakin 2011-11-23 10:12:41 PST
Created attachment 576531 [details] [diff] [review]
patch
Comment 5 Neil Deakin 2011-11-24 11:18:43 PST
Comment on attachment 576531 [details] [diff] [review]
patch

NS_IsEventTargetedAtFocusedWindow only differs from the check done at http://mxr.mozilla.org/mozilla-central/source/view/src/nsViewManager.cpp#996 (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 Masayuki Nakano [:masayuki] (Mozilla Japan) 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 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-11-30 03:14:12 PST
Comment on attachment 576531 [details] [diff] [review]
patch

if (expr) {
  stmt;
}

But based on comment from Masayuki, I'm expecting a new patch here.
Comment 8 Olli Pettay [:smaug] (vacation Aug 25-28) 2011-12-05 06:10:20 PST
Neil, will you update the patch?
Comment 9 Neil Deakin 2011-12-06 07:50:15 PST
Created attachment 579302 [details] [diff] [review]
updated patch
Comment 10 Olli Pettay [:smaug] (vacation Aug 25-28) 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) {
  stmt;
}
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 Neil Deakin 2011-12-19 10:07:41 PST
Created attachment 582868 [details] [diff] [review]
another updated patch
Comment 12 Daniel Holbert [:dholbert] 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 Bernd 2011-12-27 07:54:51 PST
another way to trigger the assert is to enter 

about:

in the url bar and hit enter.
Comment 15 Marco Bonardo [::mak] 2011-12-29 03:21:13 PST
https://hg.mozilla.org/mozilla-central/rev/f4e51ad29002
Comment 16 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 Daniel Holbert [:dholbert] 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.