1128787 - Firefox 35.0.1 crash repro (null ptr)

Status: VERIFIED FIXED

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

Description

[echo]

Attachment: ff3501-crash.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:35.0) Gecko/20100101 Firefox/35.0
Build ID: 20150122214805

Steps to reproduce:

Open http://localhost/ff3501-crash.html


Actual results:

Firefox crash


Expected results:

Nothing

Comment 1

[echo]

FF Crash Report : https://crash-stats.mozilla.com/report/index/3813db93-44ae-4bbb-b29c-e69192150203

Comment 2

[Matthias Versen [:Matti]]

Confirming in windows and linux

 2:43.45 LOG: MainThread Bisector INFO Last good revision: 1735ff2bb23e
 2:43.45 LOG: MainThread Bisector INFO First bad revision: 818f353b7aa6
 2:43.45 LOG: MainThread Bisector INFO Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1735ff2bb23e&tochange=818f353b7aa6

The inbound builds download server is down at the moment....


I suspect bug 1065835 which mentions IME in the checkin message

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sorry for the delay.

echo:

Is the testcase simplest? Looks like it's too big to understand what's the trigger.

Comment 4

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: minimum testcase

Got it.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, this is caused by odd focus management.

When designMode editor is enabled, focused elements should lose focus. However, nsFocusManager isn't kicked when focusable state of the focused element is changed.

I've tried to write test for that, however, unfortunately, -moz-user-focus doesn't work with HTML form elements (bug 379939).

Perhaps, we should kick nsFocusManager from nsHTMLDocument::EditingStateChanged():
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=e60e056a230c#2719

Any idea?

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 add test

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.1 Add test

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsEditingSession should clear focus before calling nsIEditor::PostCreate() at enabling designMode

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8590665 [details] [diff] [review]
part.2 nsEditingSession should clear focus before calling nsIEditor::PostCreate() at enabling designMode

nsIEditor::PostCreate() calls IMEStateManager::UpdateIMEState() for adjusting the IME state with its expected state. Therefore, before that, IMEStateManager::OnChangeFocus() should be called with nullptr for aContent (I.e., it means that nobody doesn't have focus in the document).

For making nsFocusManger behave so, nsEditingSession::SetupEditorOnWindow() should clear focus on the window when it starts designMode.

Of course, this cases firing blur event on the focused element. This must be better behavior than current behavior.

However, this patch doesn't adjust IME state at disabling designMode. I'd like to fix it in another bug.

Comment 10

[Neil Deakin]

Comment on attachment 8590665 [details] [diff] [review]
part.2 nsEditingSession should clear focus before calling nsIEditor::PostCreate() at enabling designMode

>+  // When designMode is turned on, focused element should lose focus.
>+  nsCOMPtr<nsIDOMHTMLDocument> htmlDoc = do_QueryInterface(doc);
>+  nsAutoString designMode;
>+  htmlDoc->GetDesignMode(designMode);
>+  if (designMode.EqualsLiteral("on")) {
>+    nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow);
>+    MOZ_ASSERT(window);
>+    nsCOMPtr<nsPIDOMWindow> focusedWindow;
>+    nsIContent* focusedContent =
>+      nsFocusManager::GetFocusedDescendant(window, false,
>+                                           getter_AddRefs(focusedWindow));
>+    if (focusedContent) {
>+      nsFocusManager* fm = nsFocusManager::GetFocusManager();
>+      if (fm) {
>+        fm->ClearFocus(window);
>+      }
>+    }
>+  }
>+

This only gets called in the one case where designMode is changed to 'on'? There seem to be other callers but I don't understand the editor initialization to know for sure.

Comment 11

[(no longer active)]

Comment on attachment 8590665 [details] [diff] [review]
part.2 nsEditingSession should clear focus before calling nsIEditor::PostCreate() at enabling designMode

Review of attachment 8590665 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this is the right fix.  We shouldn't be mucking with the focus like this just because you toggle the designMode.  I think you're just wallpapering over the real bug:

nsHTMLEditor::GetFocusedContentForIME() seems to *always* return null for designMode documents, and that change was made here: <https://hg.mozilla.org/mozilla-central/rev/73a92a915ce3#l4.22>  I'm not sure why that is, but clearly the two sides here do not agree on whether it's OK to call UpdateIMEState with a null focused content, so we need to decide who's right and change the other side accordingly.

(Note that my comments on the test may or may not be applicable to the final version of the test depending on what ends up changing.)

::: dom/events/test/test_bug1128787.html
@@ +12,5 @@
> +  <script type="application/javascript">
> +
> +  /** Test for Bug 1128787 **/
> +  SimpleTest.waitForExplicitFinish();
> +  // sometimes hits an assertion in HyperTextAccessible.cpp (Wrong in offset: 'Error')

Please file a bug to investigate and fix this, and include the bug number here.

@@ +26,5 @@
> +      var utils = SpecialPowers.getDOMWindowUtils(window);
> +      is(utils.IMEStatus, utils.IME_STATUS_ENABLED, "IME should be enabled");
> +
> +      SimpleTest.executeSoon(function () {
> +        document.designMode = "Off";

Nit: "off"

@@ +34,5 @@
> +
> +        SimpleTest.finish();
> +      });
> +    });
> +    document.designMode = "On";

Nit: "on"

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #11)
> I don't think this is the right fix.  We shouldn't be mucking with the focus
> like this just because you toggle the designMode.  I think you're just
> wallpapering over the real bug:

I agree with the fact. However, I think that we don't believe same thing is the root cause.

I believe that this is caused that nsFocusManager doesn't handle focus change at -moz-user-* is changed. At creating nsHTMLEditor because of enabling designMode or creating first contenteditable element, nsHTMLDocument inserts contenteditable.css and designmode.css.
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=e60e056a230c#2825
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=e60e056a230c#2840

In the designmode.css, all elements become read-write.
http://mxr.mozilla.org/mozilla-central/source/layout/style/designmode.css#6

Although, I don't understand well the following steps, but finally, all elements in designMode becomes not to be able to take focus (normally). So, I think that all elements in the document should lose focus at entering designMode (blur event should be fired).

> nsHTMLEditor::GetFocusedContentForIME() seems to *always* return null for
> designMode documents, and that change was made here:
> <https://hg.mozilla.org/mozilla-central/rev/73a92a915ce3#l4.22>  I'm not
> sure why that is, but clearly the two sides here do not agree on whether
> it's OK to call UpdateIMEState with a null focused content, so we need to
> decide who's right and change the other side accordingly.

The main issue of here is, IMEContentObserver fails to initialize with a document which is in designMode but has focused element (and probably it has independent selection). CreateIMEContentObserver() sends sContent which was stored at previous OnFocusChange() call. http://mxr.mozilla.org/mozilla-central/source/dom/events/IMEStateManager.cpp#1226 i.e., aContent of UpdateIMEState() is ignored, anyway.

BTW, I don't know what's the best behavior for input elements in designMode. The behavior is different on each browser :-(　On other browsers, <input type="text"> can take focus and its value can be edited from usual operation (i.e., setting focus and typing text into the field).

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, even though the CSS doesn't specify <input type="text"> in designMode isn't focusable explicitly, nsGenericHTMLEdlement::IsFocusable() returns "unfocusable".
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsGenericHTMLElement.cpp?rev=ac4464790ec4#2628

So, it's the last chance to notify nsFocusManage of modifying focus caused by enabling designMode.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> The main issue of here is, IMEContentObserver fails to initialize with a document which is in designMode but
> has focused element (and probably it has independent selection).

Um, I was confused. It's wrong. IMEContentObserver fails to initialize with an input element which is in designMode.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8590665 [details] [diff] [review]
part.2 nsEditingSession should clear focus before calling nsIEditor::PostCreate() at enabling designMode

Anyway, I believe that my approach is right since the behavioe of nsIContent::IsFocusable() is changed at toggling designMode. So, it should be correct thing to make nsFocusManager update focused element at toggling designMode.

However, this patch should check the method result before clearing focus.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Hmm, I realized that if the document already has contenteditable element, nsEditorSession::MakeWindowEditable() won't be called at enabling designMode. Additionally, -moz-user-focus will be modified after a call of MakeWindowEditable(). So, it means that nsIEditor::PostCreate() is called *before* reconstructing style data.

I think that IMEStateManager::UpdateIMEState() should ignore when aContent mismatches focused content. And after the style reconstructing, nsFocusManager should clear focus and modify IME state.

Comment 17

[(no longer active)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> I think that IMEStateManager::UpdateIMEState() should ignore when aContent
> mismatches focused content. And after the style reconstructing,
> nsFocusManager should clear focus and modify IME state.

That sounds better to me...

Note that the situation with focus and design mode right now is a mess, sorry you had to deal with this.  :(

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #17)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #16)
> > I think that IMEStateManager::UpdateIMEState() should ignore when aContent
> > mismatches focused content. And after the style reconstructing,
> > nsFocusManager should clear focus and modify IME state.
> 
> That sounds better to me...
> 
> Note that the situation with focus and design mode right now is a mess,
> sorry you had to deal with this.  :(

No problem. And if I had much time, I'd like to work on around that ;-)

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

I think that this is the best.

Inserting US stylesheet for HTML editor -> updating focus with the new style (especially, -moz-user-focus) -> making the document editable

This is different from my previous comment you agreed, but fortunately, this doesn't cause any orange.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ec19ce7437f0

Therefore, I think that this is the best approach. Could you check this? This is now not changing editor directly, but I'd like you to agree first. Then, I'll request review to Enn as focus handling expert and Smaug as content expert. (should be reviewed by dbaron too?)

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> ::: dom/events/test/test_bug1128787.html
> @@ +12,5 @@
>> +  <script type="application/javascript">
>> +
>> +  /** Test for Bug 1128787 **/
>> +  SimpleTest.waitForExplicitFinish();
>> +  // sometimes hits an assertion in HyperTextAccessible.cpp (Wrong in offset: 'Error')
> 
> Please file a bug to investigate and fix this, and include the bug number here.

I'll do this after you agree with the new patch.

Comment 21

[(no longer active)]

Comment on attachment 8591695 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

Review of attachment 8591695 [details] [diff] [review]:
-----------------------------------------------------------------

Sure, this is probably reasonable.

Comment 22

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8591695 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

Thank you, Ehsan.

Smaug, Enn:

Could you review this patch? See comment 19 for the detail.

I have a worry. This patch makes nsHTMLDocument dispatches blur event during making the window editable. This could break something even though the recursive call of EditingStateChanged() is blocked by nsAutoEditingState.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8591695 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

dbaron:

If you think that this processing order change causes some trouble, let me know.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8591695 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

So ClearFocus may dispatch events, right? Looks rather scary place to handle events. If we really need focus change here, at least use nsAutoScriptBlocker
so that the possible focus/blur events are dispatched late enough.

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

Smaug:

This is the minimum term not to create new orange. Looks like that nsHTMLEditor::BeginningOfDocument() causes selection change and it affects something at loading the document (Actually, some crash tests of docshell won't work if I make script blocker work longer.

With this change, the random assertion at running test -1.html has gone. So, this change may be necessary.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8592840 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

So with this patch if one changes designmode attribute in
focus/blur event listener (and handling the event dispatched in ClearFocus(), if I read the code correctly), we actually end up overriding the new state, since we'll set mEditingState to the older newState value since that is still on stack.
I don't see how that is in any way acceptable.

Can we somehow postpone dispatching focus/blur events?
(and could you verify ClearFocus actually ends up dispatching focus/blur)

Comment 27

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8592840 [details] [diff] [review]
> part.2 nsHTMLDocument should clear focus before making itself editable when
> designMode is enabled and it makes the focused content non-focusable
> 
> So with this patch if one changes designmode attribute in
> focus/blur event listener (and handling the event dispatched in
> ClearFocus(), if I read the code correctly), we actually end up overriding
> the new state, since we'll set mEditingState to the older newState value
> since that is still on stack.

Hmm, nested call of the method is ignored here (mEditingState == eSettingUp):
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=e60e056a230c#2725

since the block puts |nsAutoEditingState push(this, eSettingUp);|
http://mxr.mozilla.org/mozilla-central/source/dom/html/nsHTMLDocument.cpp?rev=e60e056a230c#2794

However, nsAutoEditingState and nsAutoScriptBlocker are created in same block. So, I'm not sure which is destroyed first. If the latter is first, you're right.

> I don't see how that is in any way acceptable.

Yeah, I agree.

> Can we somehow postpone dispatching focus/blur events?

No idea.

> (and could you verify ClearFocus actually ends up dispatching focus/blur)

Yes, SendFocusOrBlurEvent() is called via Blur().

Comment 28

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

How about this? This puts of firing blur event until modifying mEditableState.

FYI: -3.html doesn't test it actually since nsHTMLDocument::GetDesignMode() returns "on" or "off" from if the document node is editable. The editable flag is modified before a call of EditableStateChanged(). Therefore, even if mEditableState isn't eOff, it may return "off". I think that we should check it with inserting assertion but it could cause a lot of oranges. So, I'd like to do it in another bug.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

That is ugly, but we cannot use nsAutoScriptBlocker since we need to create nsAutoEditingState first but we need to destroy it before nsAutoScriptBlocker. (Or, should we create nsAutoEditingState::RestoreNow()?)

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8593247 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

A bit ugly, but at least a tad safer.

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8593247 [details] [diff] [review]
part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable

>+    // Adjust focused element with new style but blur event shouldn't be fired
>+    // by the last of this block.
"at the end of this block"
or something like 



So where is the focus after these changes if there is first some focused element?

Comment 32

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: part.2 nsHTMLDocument should clear focus before making itself editable when designMode is enabled and it makes the focused content non-focusable (r=smaug)

Thank you, smaug!

Enn, could you review whether this has problem for nsFocusManager?

dbaron, I'm happy if you have some comments for swapping the order of making window editable and inserting UA stylesheet for HTML editor.

Comment 33

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d9ccefea01e
https://hg.mozilla.org/integration/mozilla-inbound/rev/5ef32cc1593a

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4d9ccefea01e
https://hg.mozilla.org/mozilla-central/rev/5ef32cc1593a

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32)
> dbaron, I'm happy if you have some comments for swapping the order of making
> window editable and inserting UA stylesheet for HTML editor.

I'm not sure what you're asking.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to David Baron [:dbaron] ⏰UTC-7 from comment #35)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #32)
> > dbaron, I'm happy if you have some comments for swapping the order of making
> > window editable and inserting UA stylesheet for HTML editor.
> 
> I'm not sure what you're asking.

Although, the patch was landed yesterday, your check is still welcome.

nsHTMLDocument enabled designMode with following steps:

1. Making its window editable (creating nsHTMLEditor, setting it to its docshell)
2. Inserting UA stylesheets (designmode.css and contenteditable.css)
3. Making its presShell reconstruct the style data.

My patch changes the order to:

1. (old #2) Inserting UA stylesheets
2. (old #3) Making its presShell reconstruct the style data
3. (new) Setting auto script blocker
4. (new) adjusting focus with the new style 
5. (old #1) Making its window editable

I wonder, changing the processing order, especially the old #2, might cause some trouble by reconstructing style data before making its window editable. I.e., script blocker should be set before new #2. If so, I'll file a bug and rewrite the patches. (FYI: if we'll do so, we need to fix new oranges. IIRC, there are a lot of failure with script blocker before reconstructing the style data)

Comment 37

[Ashickur Rahman]

Florin can you help me to reproduce the bug? I am confused how to reproduce it.

Comment 38

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to Ashickur Rahman from comment #37)
> Florin can you help me to reproduce the bug? I am confused how to reproduce
> it.

Sure thing: this bug has a minimum testcase, which here is an HTML file (see it in the Attachments area before the bug description), which means that all you need to do to reproduce the bug is open the attachment on a Firefox version previous to the fix.

For any bug that you try to verify be on the lookout for 2 important things: Steps to Reproduce and/or attached testcases.

Comment 39

[Rezaul Huque Nayeem]

Reproduced the bug in 35.0.1 (2015-01-22) on windows 10 x64 by following Comment 0's instruction

Verified as fixed with latest Firefox Beta 40.0 (Build ID: 20150713153304) 

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:40.0) Gecko/20100101 Firefox/40.0

Comment 40

[Nazir Ahmed Sabbir [:NaSb]]

Successfully reproduce the bug in Firefox 35.0.1 on Linux x64 by following comment 0's instruction!

This Bug is now verified as fixed on Latest Beta 40.0b6

Build ID: 20150720220238
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:40.0) Gecko/20100101 Firefox/40.0

As it is also verified on Windows (Comment 39), Marking it as verified!