"ASSERTION: null frame" in PresShell::HandleEvent with focus, navigation

RESOLVED FIXED in mozilla12

Status

()

Core
DOM: Events
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Jesse Ruderman, Assigned: Neil Deakin (not available until Aug 9))

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla12
assertion, testcase
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox11-)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Comment 1

6 years ago
Created attachment 576414 [details]
stack trace

Comment 2

6 years ago
Is this a regression or did we used to get "null view" assertion?
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.
Created attachment 576531 [details] [diff] [review]
patch
Assignee: nobody → enndeakin
Status: NEW → ASSIGNED
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?
Attachment #576531 - Flags: review?(bugs)
Attachment #576531 - Flags: feedback?(masayuki)
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

6 years ago
Comment on attachment 576531 [details] [diff] [review]
patch

if (expr) {
  stmt;
}

But based on comment from Masayuki, I'm expecting a new patch here.
Attachment #576531 - Flags: review?(bugs)

Comment 8

6 years ago
Neil, will you update the patch?
Created attachment 579302 [details] [diff] [review]
updated patch
Attachment #576531 - Attachment is obsolete: true
Attachment #576531 - Flags: feedback?(masayuki)
Attachment #579302 - Flags: review?(bugs)
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()?
Attachment #579302 - Flags: review?(bugs) → review-

Updated

6 years ago
tracking-firefox11: --- → ?
Created attachment 582868 [details] [diff] [review]
another updated patch
Attachment #579302 - Attachment is obsolete: true
Attachment #582868 - Flags: review?(bugs)

Updated

6 years ago
Attachment #582868 - Flags: review?(bugs) → review+
(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!)
OS: Mac OS X → All
Hardware: x86_64 → All

Comment 13

6 years ago
another way to trigger the assert is to enter 

about:

in the url bar and hit enter.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4e51ad29002
Flags: in-testsuite-
https://hg.mozilla.org/mozilla-central/rev/f4e51ad29002
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
It's unclear that this is a common user scenario. Please re-nominate if that's not the case.
tracking-firefox11: ? → -
Neil, do you know if this bug has any effects in opt builds?
You need to log in before you can comment on or make changes to this bug.