Closed Bug 704758 Opened 13 years ago Closed 12 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla12
Tracking Status
firefox11 - ---

People

(Reporter: jruderman, Assigned: enndeakin)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 2 obsolete files)

Attached file 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
Attached file stack trace
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.
Attached patch patch (obsolete) — Splinter Review
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 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)
Neil, will you update the patch?
Attached patch updated patch (obsolete) — Splinter Review
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-
Attachment #579302 - Attachment is obsolete: true
Attachment #582868 - Flags: review?(bugs)
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
another way to trigger the assert is to enter 

about:

in the url bar and hit enter.
https://hg.mozilla.org/mozilla-central/rev/f4e51ad29002
Status: ASSIGNED → RESOLVED
Closed: 12 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.
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.