Closed Bug 1218032 Opened 8 years ago Closed 8 years ago

Ctrl-b while editing a mana page crashes the tab (crash in nsContentIterator::NextNode)

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla45
Tracking Flags:
Tracking Status
firefox44 + fixed
firefox45 + fixed

People

(Reporter: jimm, Assigned: masayuki)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(9 files, 5 obsolete files)

The result of caret_on_presshell_reinit-2.html in e10s mode
5.72 KB, image/png
Details
Reference of caret_on_presshell_reinit-2.html
5.72 KB, image/png
Details
part.1 nsContentIterator should warn if unexpected case occurs (r=smaug)
10.76 KB, patch
Details | Diff | Splinter Review
part.2 Make sending focus notification to IME async-aware
10.97 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.3 IMEContentObserver should notify IME of anything without script runner (r=smaug)
15.01 KB, patch
Details | Diff | Splinter Review
part.4 Notify IMEContentObserver of enough safe timing to notify IME of something (r=smaug)
16.05 KB, patch
Details | Diff | Splinter Review
part.5 Mark caret_on_presshell_reinit-2.html as fuzzy in e10s mode (r=smaug)
2.24 KB, patch
Details | Diff | Splinter Review
part.1 Backout bug 1215816 from Aurora
1.38 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part.2 Backout bug 1215798 from Aurora
2.67 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
Editing a mana page, I highlight a word and hit ctrl-b. This is reproducible.

https://crash-stats.mozilla.com/report/index/48a10889-332d-4965-9d11-e07372151023
Crash Signature: [@ nsContentIterator::NextNode ]
Regression from the recent changes?
Flags: needinfo?(masayuki)
Looks like same as bug 1217877. I guess that the trigger is the change of NodeIsInTraversalRange()?
Flags: needinfo?(masayuki)
Okay, I sometimes reproduced this bug in a mana page. I try to investigate this bug today.
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
I have a reproducible testcase for that crash:

1. Open https://mana.mozilla.org/wiki/pages/editpage.action?pageId=26414281
2. Press Ctrl+F and enter "Windows"
3. Press return twice

After those steps Firefox always crashes for me. Here one of those crash reports: bp-beeac2b7-023d-4d45-9ca4-8ca982151029
Severity: normal → critical
Keywords: crash
OS: Unspecified → All
Hardware: Unspecified → All
For me, I had this crash as well. But my STR was:
- editing a table
- highlighted two rows of the table
- tapped 'delete' and it crashed

It consistently happened. Worked fine if I was just highlighting one row (and tapped delete) - it only crashed if I highlighted more than 1 row.
(In reply to Karen Rudnitski [:kar] from comment #10)
> For me, I had this crash as well. But my STR was:
> - editing a table
> - highlighted two rows of the table
> - tapped 'delete' and it crashed
> 
> It consistently happened. Worked fine if I was just highlighting one row
> (and tapped delete) - it only crashed if I highlighted more than 1 row.

I cannot create a simple testcase with this behavior too. If we could reproduce the bug with simple testcase, I'd add it to automated tests, though.
http://jsfiddle.net/d_toybox/p18huwr4/
I can systematically reproduce the crash with WordPress: select a word with the visual editor enabled, apply bold, click to remove bold, crash.

Both with Nightly 45.0a1 (2015-11-04) and Dev Edition 44.0a2 (2015-11-05). On Nightly with both e10s enabled and disabled.

https://crash-stats.mozilla.com/report/index/a243058f-9208-4146-984e-de0ce2151105
https://crash-stats.mozilla.com/report/index/ff4f7e6f-d025-4e81-812e-f0f9d2151105

Given that this bug is linked from the Crash Report site, maybe we should amend the subject and raise severity.
See Also: → 1222460
This is not a part of the fix of this bug, though.

nsContentIterator assumes never to occur unexpected situation in a lot of place. The crash is one of the results of such code.

This patch outputs warnings when nsContentIterator meets unexpected case. This patch must help to debug it.
Attachment #8684492 - Flags: review?(bugs)
According to the warnings of the part.1 and the crash reports, the root cause is that nsContentIterator doesn't assume that it won't be used with removed nodes from the document. However, current IMEContentObserver may do that. Immediately after methods of nsIMutationObserver are called, script blocker is destroyed. This may kick IMENotificationSender.

When content removed and the selection is in the removed content, IMEContentObserver receives selection change notification but the range is still in the removed node. Then, kicked IMENotificationSender tries to retrieve the selection ranges with removed nodes. At this time, nsContentIterator crashes.

For solving this root cause, we should *NOT* use script blocker to kick IMENotificationSender since we are learning it's dangerous immediately after some contents are removed. So, we should use NS_DispatchToCurrentThread() instead of nsContentUtils::AddScriptRunner().

This patch fixes an existing bug. Even with current IMEContentObserver, NOTIFY_IME_OF_FOCUS may be sent with NS_DispatchToCurrentThread(). However, Init() doesn't assume that it may be performed asynchronously. In TSF mode on Windows, nsIWidget returns different nsIMEUpdatePreference whether IME has focus or not. Therefore, before sending NOTIFY_IME_OF_FOCUS, ObserveEditableNode() starts to observe the contents with wrong preference.

This patch fixes this *another* bug before making IMENotificationSender performed with NS_DispatchToCurrentThread().
Attachment #8684500 - Flags: review?(bugs)
This is the main fix of this bug.

For the following patches, this adds mQueuedSender instead of removing mIsFlushingPendingNotifications.

And now, AsyncMergeableNotificationsFlusher class isn't necessary.
Attachment #8684502 - Flags: review?(bugs)
According to some developers who I asked about the order of dispatched gecko event vs. coming next event in IRC, following native input event may be handled before dispatched gecko event. In such case, IME may be confused. Especially when first key event after focus change comes before focus notification.

For preventing such situation, PresShell should flush the pending notifications to IME before going back to widget after a widget event is handled.

Additionally, IMEStateManager should flush them too immediately after creating an instance of IMEContentObserver and perhaps when it's safe. This is still hacky but we need to do this especially for some automated tests which checking NOTIFY_IME_OF_FOCUS... Anyway, we should try to fix this issue in another bug when we have much time or we have some bug reports caused by this.
Attachment #8684503 - Flags: review?(bugs)
I cannot fix this bug here. According to the screenshot, this is caused by changing the timing to call of nsIPresShell::FlushPendingNotfications() from ContentEventHandler.

Let's make this fuzzy for now. (I'll post the images as attachment)
Attachment #8684504 - Flags: review?(bugs)
The patches are risky but this patch may fix some other bugs too since there are a lot of odd warnings in e10s mode. So, I think that we should uplift them to Aurora since it's still in very early stage of current cycle.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (offline: 10/31 - 11/3, JST) from comment #24)
> Created attachment 8684504 [details] [diff] [review]
> part.5 Mark caret_on_presshell_reinit-2.html as fuzzy in e10s mode
> 
> I cannot fix this bug here. According to the screenshot, this is caused by
> changing the timing to call of nsIPresShell::FlushPendingNotfications() from
> ContentEventHandler.
> 
> Let's make this fuzzy for now. (I'll post the images as attachment)

FYI: This failure occurs randomly (but a little bit high frequency 60% or more). When this failure occurs, there is an assertion:

> [Child 1935] ###!!! ASSERTION: Can only call this on frames that have been reflowed: '!(GetStateBits() & NS_FRAME_FIRST_REFLOW) || (GetParent()->GetStateBits() & NS_FRAME_TOO_DEEP_IN_FRAME_TREE)', file /builds/slave/try-l64-d-00000000000000000000/build/src/layout/generic/nsTextFrame.cpp, line 2710

We meet this assertion when I change IMEContentObserver behavior. I guess we should investigate why this occurs.
Do the patches here perhaps fix also bug 1222460?
(In reply to Olli Pettay [:smaug] from comment #30)
> Do the patches here perhaps fix also bug 1222460?

probably since it's exactly same stack.
Comment on attachment 8684492 [details] [diff] [review]
part.1 nsContentIterator should warn if unexpected case occurs

Might be worth to check that we don't start to get tons of warnings while
running mochitests. So, check tryserver logs, and if there are cases where
warning is printed very often, remove those cases.

   while (numChildren) {
     nsIContent* child = node->GetChildAt(--numChildren);
 
     if (aIndexes) {
       // Add this node to the stack of indexes
       aIndexes->AppendElement(numChildren);
     }
-    numChildren = child->GetChildCount();
+    numChildren = static_cast<int32_t>(child->GetChildCount());
+    NS_WARN_IF(numChildren < 0);
     node = child;

     
That looks like a case which may happen very often so I'd remove it.

     if (node->HasChildren()) {
       nsIContent* firstChild = node->GetFirstChild();
+      NS_WARN_IF(!firstChild);
Shouldn't that be more like MOZ_ASSERT


   if (sibling != node) {
     // someone changed our index - find the new index the painful way
     indx = parent->IndexOf(node);
+    NS_WARN_IF(indx  < 0);
extra space before <
Attachment #8684492 - Flags: review?(bugs) → review+
I confirmed that this is a regression of bug 1215798.
Blocks: 1215798
Keywords: regression
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #21)
> For solving this root cause, we should *NOT* use script blocker to kick
> IMENotificationSender since we are learning it's dangerous immediately after
> some contents are removed. So, we should use NS_DispatchToCurrentThread()
> instead of nsContentUtils::AddScriptRunner().

This sounds very regression risky. Sure, there can be other script runners too n stack, but it is even more likely
that there other runnables in event loop waiting to be processed.


I don't quite understand reason for the change. Looking at the patch...
> I don't quite understand reason for the change. Looking at the patch...

The reason is, immediately after unbinding a removed node from tree (after a call of nsIMutationObserver::ContentRemoved()), a script blocker is destroyed from the stack. Then, IMENotificationSender is kicked. However, selection hasn't been moved to existing node in the document yet, i.e., selection node is still removed node, nsContentIterator hits a node whose parent is nullptr.

If you don't like to uplift them, we can just backout the patches (bug 1215798 and bug 1215816).
Comment on attachment 8684503 [details] [diff] [review]
part.4 Notify IMEContentObserver of enough safe timing to notify IME of something

>+  AutoRestore<bool> ruuning(mIsRunning);
running



>+    if (!mIsDestroying && aIsHandlingNativeEvent) {
>+      // Ensure that notifications to IME should be sent before getting next
>+      // native event from the event queue.
>+      // XXX Should we check the event message or event class instead of
>+      //     using aIsHandlingNativeEvent?
>+      manager->TryToFlushPendingNotificationsToIME();
>+    }
I wonder if we need any check at all. Non native events aren't dispatched often via presshell.
But fine to leave it this way too and possibly remove the param in a followup if it is considered safe.
Attachment #8684503 - Flags: review?(bugs) → review+
Or, adding nullptr check into nsContentIterator, but probably, it causes other symptom.
> Non native events aren't dispatched often via presshell.

Unfortunately, input event is fired via PresShell :-(
Comment on attachment 8684504 [details] [diff] [review]
part.5 Mark caret_on_presshell_reinit-2.html as fuzzy in e10s mode

Not too happy with this. I wonder if we could just change the test to wait longer to get the right painting or so.
Attachment #8684504 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #40)
> Comment on attachment 8684504 [details] [diff] [review]
> part.5 Mark caret_on_presshell_reinit-2.html as fuzzy in e10s mode
> 
> Not too happy with this. I wonder if we could just change the test to wait
> longer to get the right painting or so.

Yeah, I tried with setTimeout(, 0); at removing the class attribute of <html>, but not reducing the frequency. I think that I should investigate the cause of the assertion after I fix urgent bugs.
what about requestAnimationFrame + setTimeout(0)? That way timeout would happen right after the next paint.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #36)
> > I don't quite understand reason for the change. Looking at the patch...
> 
> The reason is, immediately after unbinding a removed node from tree (after a
> call of nsIMutationObserver::ContentRemoved()), a script blocker is
> destroyed from the stack. Then, IMENotificationSender is kicked. However,
> selection hasn't been moved to existing node in the document yet, i.e.,
> selection node is still removed node, nsContentIterator hits a node whose
> parent is nullptr.
Would it be possible to add a null check to IMENotificationSender? Some, IsInComposedDoc() check?


> If you don't like to uplift them, we can just backout the patches (bug
> 1215798 and bug 1215816).

nah, I think better to just try to fix the issue.
(In reply to Olli Pettay [:smaug] from comment #43)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #36)
> > > I don't quite understand reason for the change. Looking at the patch...
> > 
> > The reason is, immediately after unbinding a removed node from tree (after a
> > call of nsIMutationObserver::ContentRemoved()), a script blocker is
> > destroyed from the stack. Then, IMENotificationSender is kicked. However,
> > selection hasn't been moved to existing node in the document yet, i.e.,
> > selection node is still removed node, nsContentIterator hits a node whose
> > parent is nullptr.
> Would it be possible to add a null check to IMENotificationSender? Some,
> IsInComposedDoc() check?

I did it with a new method of ContentEventHandler, which checks if mFirstSelectionRange is in the document. However, every flush can change selection range state. So, checking before a call of UpdateSelectionCache() didn't make sense...

And I also tries to make OnQuerySelectedText() return error if the selection range isn't in the document. But it caused odd failure of bc7:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3df80805660a
The patch is this:
https://hg.mozilla.org/try/rev/e9045f4f0eb8
I don't understand why such failure occurred.
(In reply to Olli Pettay [:smaug] from comment #42)
> what about requestAnimationFrame + setTimeout(0)? That way timeout would
> happen right after the next paint.

Is this patch you expected?
https://hg.mozilla.org/try/rev/31bd1bf48c30
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #46)
> (In reply to Olli Pettay [:smaug] from comment #42)
> > what about requestAnimationFrame + setTimeout(0)? That way timeout would
> > happen right after the next paint.
> 
> Is this patch you expected?
> https://hg.mozilla.org/try/rev/31bd1bf48c30

Hmm, it becomes orange:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=31bd1bf48c30
Comment on attachment 8684500 [details] [diff] [review]
part.2 Make sending focus notification to IME async-aware

>   // Some change events may wait to notify IME because this was being
>   // initialized.  It is the time to flush them.
>-  FlushMergeableNotifications();
>+  if (NeedsToNotifyIMEOfSomething()) {
>+    FlushMergeableNotifications();
>+  }
>+}
Would it make sense to have 
if (!NeedsToNotifyIMEOfSomething()) {
  return;
}
in FlushMergeableNotifications()? 

>+
>+void
>+IMEContentObserver::OnIMEReceiveFocus()
Perhaps call this OnIMEReceivedFocus

>@@ -1413,20 +1438,32 @@ IMEContentObserver::IMENotificationSende
>   MOZ_ASSERT(mIMEContentObserver->mIsFlushingPendingNotifications);
> 
>   // NOTE: Reset each pending flag because sending notification may cause
>   //       another change.
> 
>   if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
>     mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet = false;
>     SendFocusSet();
>+    mIMEContentObserver->mIsFlushingPendingNotifications = false;
>+    // If it's not safe to notify IME of focus, SendFocusSet() sets
>+    // mNeedsToNotifyIMEOfFocusSet true again.  For guaranteeing to send the
>+    // focus notification later,  we should put a new sender into the queue.
Ah, so usually mNeedsToNotifyIMEOfFocusSet isn't set to true again, right?

>+    if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
>+      MOZ_LOG(sIMECOLog, LogLevel::Debug,
>+        ("IMECO: 0x%p IMEContentObserver::IMENotificationSender::Run(), "
>+         "posting AsyncMergeableNotificationsFlusher to current thread", this));
>+      RefPtr<AsyncMergeableNotificationsFlusher> asyncFlusher =
>+        new AsyncMergeableNotificationsFlusher(mIMEContentObserver);
>+      NS_DispatchToCurrentThread(asyncFlusher);
>+      return NS_OK;
>+    }
I can't figure out when we're suppose to run this code and what code does this prevents running synchronously and why 
the FlushMergeableNotifications in IMEContentObserver::AsyncMergeableNotificationsFlusher::Run()
doesn't cause us recreating an AsyncMergeableNotificationsFlusher again.
Could you please explain, also in the code.

>@@ -1500,16 +1537,18 @@ IMEContentObserver::IMENotificationSende
> 
>   MOZ_RELEASE_ASSERT(mIMEContentObserver->mSendingNotification ==
>                        NOTIFY_IME_OF_NOTHING);
>   mIMEContentObserver->mSendingNotification = NOTIFY_IME_OF_FOCUS;
>   IMEStateManager::NotifyIME(IMENotification(NOTIFY_IME_OF_FOCUS),
>                              mIMEContentObserver->mWidget);
>   mIMEContentObserver->mSendingNotification = NOTIFY_IME_OF_NOTHING;
> 
>+  mIMEContentObserver->OnIMEReceiveFocus();
Could you explain why we can notify IME about focus even before we observe anything?
OnIMEReceiceFocus calls ObserveEditableNode. 
This could use a comment in the code.


Ok, I'm starting to see this not being too scary after all, but would like to see some more comments, in code and as a bugzilla comment too. This is rather complicated setup here.
Attachment #8684500 - Flags: review?(bugs) → review-
Comment on attachment 8684502 [details] [diff] [review]
part.3 IMEContentObserver should notify IME of anything without script runner

> IMEContentObserver::IMENotificationSender::Run()
> {
>-  MOZ_ASSERT(mIMEContentObserver->mIsFlushingPendingNotifications);
>+  MOZ_ASSERT(mIMEContentObserver->mQueuedSender);
> 
>   // NOTE: Reset each pending flag because sending notification may cause
>   //       another change.
> 
>   if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
>     mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet = false;
>     SendFocusSet();
>-    mIMEContentObserver->mIsFlushingPendingNotifications = false;
>+    mIMEContentObserver->mQueuedSender = nullptr;
>     // If it's not safe to notify IME of focus, SendFocusSet() sets
>     // mNeedsToNotifyIMEOfFocusSet true again.  For guaranteeing to send the
>     // focus notification later,  we should put a new sender into the queue.
>     if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
>       MOZ_LOG(sIMECOLog, LogLevel::Debug,
>         ("IMECO: 0x%p IMEContentObserver::IMENotificationSender::Run(), "
>-         "posting AsyncMergeableNotificationsFlusher to current thread", this));
>-      RefPtr<AsyncMergeableNotificationsFlusher> asyncFlusher =
>-        new AsyncMergeableNotificationsFlusher(mIMEContentObserver);
>-      NS_DispatchToCurrentThread(asyncFlusher);
>+         "posting IMENotificationSender to current thread", this));
>+      mIMEContentObserver->mQueuedSender =
>+        new IMENotificationSender(mIMEContentObserver);
>+      NS_DispatchToCurrentThread(mIMEContentObserver->mQueuedSender);
>       return NS_OK;
Note to my self: Ok, this doesn't enter to endless runnable posting because 
SendFocusSet returns early if !CanNotifyIME.
Attachment #8684502 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #48)
> >+    mIMEContentObserver->mIsFlushingPendingNotifications = false;
> >+    // If it's not safe to notify IME of focus, SendFocusSet() sets
> >+    // mNeedsToNotifyIMEOfFocusSet true again.  For guaranteeing to send the
> >+    // focus notification later,  we should put a new sender into the queue.
> Ah, so usually mNeedsToNotifyIMEOfFocusSet isn't set to true again, right?

Right. I explain it with comment in the code explicitly.

> >+    if (mIMEContentObserver->mNeedsToNotifyIMEOfFocusSet) {
> >+      MOZ_LOG(sIMECOLog, LogLevel::Debug,
> >+        ("IMECO: 0x%p IMEContentObserver::IMENotificationSender::Run(), "
> >+         "posting AsyncMergeableNotificationsFlusher to current thread", this));
> >+      RefPtr<AsyncMergeableNotificationsFlusher> asyncFlusher =
> >+        new AsyncMergeableNotificationsFlusher(mIMEContentObserver);
> >+      NS_DispatchToCurrentThread(asyncFlusher);
> >+      return NS_OK;
> >+    }
> I can't figure out when we're suppose to run this code and what code does
> this prevents running synchronously and why 
> the FlushMergeableNotifications in
> IMEContentObserver::AsyncMergeableNotificationsFlusher::Run()
> doesn't cause us recreating an AsyncMergeableNotificationsFlusher again.
> Could you please explain, also in the code.

I don't have idea of such actual scenario. Even with these patches, selection may be in removed node. So, there are sill existing some mystery. So, IsSafeToNotifyIME() *might* return false even in this case.

> >@@ -1500,16 +1537,18 @@ IMEContentObserver::IMENotificationSende
> > 
> >   MOZ_RELEASE_ASSERT(mIMEContentObserver->mSendingNotification ==
> >                        NOTIFY_IME_OF_NOTHING);
> >   mIMEContentObserver->mSendingNotification = NOTIFY_IME_OF_FOCUS;
> >   IMEStateManager::NotifyIME(IMENotification(NOTIFY_IME_OF_FOCUS),
> >                              mIMEContentObserver->mWidget);
> >   mIMEContentObserver->mSendingNotification = NOTIFY_IME_OF_NOTHING;
> > 
> >+  mIMEContentObserver->OnIMEReceiveFocus();
> Could you explain why we can notify IME about focus even before we observe
> anything?

Because if IME (or ContentCacheInChild) may retrieve content, selection and position information when they receive NOTIFY_IME_OF_FOCUS. When the content is changed *after* that, widget need to retrieve the information again. Therefore, I think that it's enough to observe the nodes after sending it.

However, it might be late in some cases. Flushing layout with eQueryContent event may cause different notification. So, during sending NOTIFY_IME_OF_FOCUS, two or more eQueryContent events may cause changes before IMEContentObserver observes the content.

Anyway, we should change the behavior later. These patches change the behavior of IMEContentObserver a lot.

> Ok, I'm starting to see this not being too scary after all, but would like
> to see some more comments, in code and as a bugzilla comment too. This is
> rather complicated setup here.

Yeah, check this new patch.
Attachment #8684500 - Attachment is obsolete: true
Attachment #8684618 - Flags: review?(bugs)
I guess that OnIMEReceivedFocus() should be called when first call of HandleQueryContentEvent() during sending NOTIFY_IME_OF_FOCUS too.
Comment on attachment 8684618 [details] [diff] [review]
part.2 Make sending focus notification to IME async-aware

>+    // When this is called first time, IME has not received NOTIFY_IME_OF_FOCUS
>+    // yet since NOTIFY_IME_OF_FOCUS will be sent to widget asynchronously.
>+    // So, we need to do nothing here.  After NOTIFY_IME_OF_FOCUS is sent
>+    // actually,
Maybe "After NOTIFY_IME_OF_FOCUS has been sent, OnIMEReceivedFocus() ..."

>+ OnIMEReceivedFocus() will be called and starting to observe
>+    // content, selection and/or position changes which are requested by
>+    // the widget having focus.
perhaps.

"will be called and content, selection and/or position changes will be observed."


>+  // When this is called when editor is reframed (i.e., the root editable node
When this is called after editor reframing ....
Attachment #8684618 - Flags: review?(bugs) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cda7977a8f1b346c9334b096a94389ff09d8381
Bug 1218032 part.1 nsContentIterator should warn if unexpected case occurs r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/2cb75cbd2d6b2c3668cdc5b51a59c131ce1434a1
Bug 1218032 part.2 Make sending focus notification to IME async-aware r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/f691194085bd983eec04f25c55a2c02b7e7ff4d1
Bug 1218032 part.3 IMEContentObserver should notify IME of anything without script runner r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/4c9308a551ba0ca6b8385df7e86f8f3a057919b9
Bug 1218032 part.4 Notify IMEContentObserver of enough safe timing to notify IME of something r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/762b1f35b89e0f5ba6b3887f4d67582e0c28bd23
Bug 1218032 part.5 Mark caret_on_presshell_reinit-2.html as fuzzy in e10s mode r=smaug
Smaug:

Thank you for your review. How do you think that should we uplift the patches to Aurora or backout the patches for bug 1215798 and bug 1215816 from Aurora?
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #58)
> Smaug:
> 
> Thank you for your review. How do you think that should we uplift the
> patches to Aurora or backout the patches for bug 1215798 and bug 1215816
> from Aurora?

If the bugs for which bug 1215798 and bug 1215816 were filed aren't in Aurora, backing those out sounds safer. (looks like bug 1213589 hasn't landed yet anywhere.)
Flags: needinfo?(bugs)
Comment 62 is a followup which I think is correct, swapping the order of the manifest conditions so that the skip-if comes last, because after you landed caret_on_presshell_reinit-2.html was running on B2G emulator, and intermittently failing.
(In reply to Phil Ringnalda (:philor) from comment #63)
> Comment 62 is a followup which I think is correct, swapping the order of the
> manifest conditions so that the skip-if comes last, because after you landed
> caret_on_presshell_reinit-2.html was running on B2G emulator, and
> intermittently failing.

Thank you. Although, it sounds odd rule to me.
Indeed. Once you know it exists, "the last matching condition is the only one which applies" is much easier to remember, and was probably easier to implement, than precedence rules would be, but intuitively you expect that skip-if should have higher precedence than fuzzy-if.
Thank you Masayuki for fixing this nasty crasher. With yesterdays Nightly I can verify that Firefox no longer crashes when searching for something while editing a page on Mana.
Let's backout them.
Attachment #8686023 - Flags: review?(bugs)
Uh, can we please put those backout patches to some other bug.
Attachment #8686023 - Flags: review?(bugs) → review+
Comment on attachment 8686024 [details] [diff] [review]
part.2 Backout bug 1215798 from Aurora

So the backout patches look good, but could we deal the backout in some other bug.
Attachment #8686024 - Flags: review?(bugs) → review+
Blocks: 1224101
See Also: → 1224101
No longer blocks: 1224101
Depends on: 1224101
Depends on: 1224233
Tracked for FF44 and FF45. This was mentioned in the channel meeting today and last week.
tracking-firefox44: ? → +
tracking-firefox45: ? → +
Updating status-firefox44 to fixed as bug 1224101 took care of the back out needed to fix this.
status-firefox44: affected → fixed
Depends on: 1230660
No longer depends on: 1258576
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.