[Messages][Text Selection] Carets appear above the selection of a vcf file's text in the messages app

RESOLVED FIXED in Firefox 39

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: dharris, Assigned: jeremychen)

Tracking

unspecified
mozilla39
ARM
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-b2g:2.2+, firefox37 wontfix, firefox38 wontfix, firefox39 fixed, b2g-v2.2 fixed, b2g-master fixed)

Details

(Whiteboard: [3.0-Daily-Testing], )

Attachments

(11 attachments, 5 obsolete attachments)

40.12 KB, text/plain
Details
5.25 MB, video/mp4
Details
137.39 KB, text/plain
Details
1.93 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.42 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.11 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.55 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.07 KB, patch
roc
: review+
Details | Diff | Splinter Review
32.68 KB, image/png
Details
50.96 KB, image/png
Details
28 bytes, text/plain
Details
Description:
The carats for text selection appear above the selected text when selecting a vcf file within a message thread


Repro Steps:
1) Update a Flame to 20150306010207
2) Open Messages app> Open a message thread, or start a new one
3) Send a message with an attached vcf file (contact info)
4) Long press on the sent vcf file
5) Tap "Select text"


Actual:
The carats appear above the selected text


Expected:
Carats appear below the selected text and are indicating where the selection ends at the tip of the carat

Environmental Variables:
Device: Flame 3.0 (319mb)(Kitkat)(Full Flash)
Build ID: 20150306010207
Gaia: 7a91c16bfa348be8b25e09719178efa051512988
Gecko: 0189941a3fd5
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 39.0a1 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:39.0) Gecko/39.0 Firefox/39.0

Repro frequency: 10/10
See attached: Logcat, Video - http://youtu.be/czX7GHOTFEs
Adding QAwanted for a logcat
_________________________________________________________________________

This issue DOES occur on Flame 2.2

The carats appear above the selected text

Device: Flame 2.2 (319mb)(Kitkat)(Full Flash)
Build ID: 20150305002528
Gaia: 89af288bad6751248ff84504fa898206fee127fe
Gecko: 6d8d294aa8f3
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 37.0 (Master)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0


This issue does NOT occur on Flame 2.1

The option to share a contact via messages was no yet implemented

Device: Flame 2.1 (319mb)(Kitkat)(Full Flash)
Build ID: 20150302001220
Gaia: 5d3479fdd438412adee4452720856b6b771fe5cd
Gecko: 9bf4c663241f
Gonk: e7c90613521145db090dd24147afd5ceb5703190
Version: 34.0 (2.1)
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
Keywords: qawanted
Summary: Messages][Text Selection] Carats appear above the selection of a vcf file's text in the messages app → Messages][Text Selection] Carets appear above the selection of a vcf file's text in the messages app
Gerry, can you take a look at this please?
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker) → needinfo?(gchang)
Requested logcat for qawanted.
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
Keywords: qawanted
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
Summary: Messages][Text Selection] Carets appear above the selection of a vcf file's text in the messages app → [Messages][Text Selection] Carets appear above the selection of a vcf file's text in the messages app
QA: can you check if this happens with other attachment types, please ?
Keywords: qawanted
Posted video video
This problem can be repro on latest build of Flame 2.2/Flame 3.0 with attachments of video/music/gallery/camera/vcf:
1. Long press the message with video/music/gallery/camera/vcf file attachment to select text, the start and end carats appear above the selected text.
2. Long press the message both with video/music/gallery/camera attachment and body text to select text, only the start carat appears above the selected text.
3. After selecting the message, there's an unwanted blue block displayed on the attachment icon.
See attachment: Flame3.0_video.MP4 & logcat_flame3.0_1135.txt
Occurrence time: 11:35
Rate: 5/5

Flame 2.2 build:
Build ID               20150308002503
Gaia Revision          166491b92278dc9e648f8d49ab02d9ca00d74421
Gaia Date              2015-03-06 18:26:27
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/a48af0b5a6e4
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.052515
Firmware Date          Sun Mar  8 05:25:25 EDT 2015
Bootloader             L1TC000118D0

Flame 3.0 build:
Build ID               20150308160204
Gaia Revision          fea83511df9ccba64259346bc02ebf2c417a12c2
Gaia Date              2015-03-08 06:36:28
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab4a81e4457
Gecko Version          39.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150308.192120
Firmware Date          Sun Mar  8 19:21:31 EDT 2015
Bootloader             L1TC000118D0
Posted file logcat
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+]
Keywords: qawanted
Jeremy, please help to check the calculation of end caret's position. It looks like the range is correct but the position to draw end caret is wrong.
Flags: needinfo?(jeremychen)
For the starting caret position, please see bug 1127648 that we might have no plan to fix it in the near future because of bug 1127648 comment 8 and bug 1127648 comment 9. But there is another serious issue that vcard's file name should not be selectable. Will file another bug for this.
See Also: → 1127648
QA Whiteboard: [QAnalyst-Triage+][MGSEI-Triage+] → [QAnalyst-Triage+][MGSEI-Triage+][COM=Text Selection]
Flags: needinfo?(gchang)
It looks like when sending a message with attachments inside, an extra <span> node is created as text node's parent. The extra frame level leads the mis-calculation for end caret position. Both endFrame and endOffset seem wrong in this case.
Flags: needinfo?(jeremychen)
Assignee: nobody → jeremychen
Set as 2.2 blocker because of it's related to copypaste feature
blocking-b2g: --- → 2.2+
I try to use DONT_DO_THIS_YET code in [1]. It seems that the function was initially designed to be called recursively if the node contains children. This patch can fix this bug, but few fails occur on try server (please see try results in [2]).

[1] https://dxr.mozilla.org/mozilla-central/source/layout/generic/nsSelection.cpp#1858
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=440b868ff2cd
Attachment #8577855 - Attachment description: wip-bug1140625-v2.patch → Add iterative call in SelectionCarets.cpp to find the right caret frame for node offset. (wip v2)
Attachment #8577855 - Attachment filename: wip-bug1140625-v2.patch → iterative call in SelectionCarets.cpp to find the right caret frame for node offset. (wip v2).patch
Attachment #8577853 - Attachment description: wip-bug1140625-v1.patch → Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1)
Attachment #8577853 - Attachment filename: wip-bug1140625-v1.patch → Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1).patch
Comment hidden (obsolete)
Comment on attachment 8577853 [details] [diff] [review]
Add recursive call in nsSelection.cpp to find the right caret frame for node offset. (wip v1)

I can keep working on wip-v1.patch and try to fix the try errors. An alternative would be patching SelectionCarets.cpp only, like wip-v2.path does, to avoid the possibility to affect other features in Gecko.

roc, can you take a look at this please? I'm not sure which way would be more appropriate.
Attachment #8577853 - Flags: feedback? → feedback?(roc)
Component: Gaia::SMS → Selection
Product: Firefox OS → Core
Jeremy, have you got a simple testcase for this bug?
An simple testcase is in https://jsfiddle.net/eqx37fps/8/. Enable caret and refresh the test web page. It can be seen that both cases can select text with right range. But caret in the case with an extra <span> node level would be wrongly positioned.
Flags: needinfo?(jeremychen)
I think the primary issue here is that in nsFrameSelection::GetFrameForNodeOffset, for the lastRange, the last child of the <p> element is a TextNode with no frame, which causes nsFrameSelection::GetFrameForNodeOffset to fall back to code with the comment
          // If we're at a collapsed whitespace content node (which
          // does not have a primary frame), just use the original node
          // to get the frame on which we should put the caret.
which then completely fails.

The TextNode has no frame because as an optimization, we avoid creating frames for whitespace-only textnodes at the end of a block, when they can't have any layout or rendering effect. If that optimization was disabled I think the current nsFrameSelection::GetFrameForNodeOffset code would work, because we'd place the caret based on the location of that whitespace-only text node, which is at the end of the block.

There are a couple of things we could do here. One is to do something like what GetTextFrameForContent does in nsRange: disable the textnode optimization for the specific textnode we care about, and ensure a textframe gets created for that node. That's a bit tricky in SelectionCarets since flushing layout means all the nsIFrame* pointers we already have could be invalid, so call callers to nsFrameSelection::GetFrameForNodeOffset would have to be carefully audited to handle flushing.

It's probably better to fix nsFrameSelection::GetFrameForNodeOffset to compute the right results without flushing. Instead of the patches here, I think the right thing to do is fix the case where textNode has no frame. In that case I think we should recursively call GetFrameForNodeOffset on the previous child node, passing in its child count as the offset to find its end. Or, if we're searching forward for the start range and we find a missing text frame, recursively call GetFrameForNodeOffset on the next child after the text node, passing in 0 as the offset.

While we're here, the call to mPresShell->FlushPendingNotifications(Flush_Layout) in SelectionCarets::UpdateSelectionCarets is in the wrong place. It has to be before we get rootFrame, since it could destroy rootFrame and replace it with some other object. Please fix that.
Hi roc, I'm a bit confused with what you said in comment 18.

In the simple testcase given in comment 17, two cases are given, the abnormal one on top and the normal one on bottom. In the abnormal case, the last child of <p> is SpanNode, not TextNode. Since SpanNode is not a TextNode, it won't even pass the isTextNode condition to get to the whitespace-only textnode case as you mentioned.

According to the dumped frame tree, it looks like nsFrameSelection::GetFrameForNodeOffset would return SpanNode frame, not the TextNode frame, since TextNode is no longer a child of <p>. Therefore, I think the problem is that the unexpectedly inserted SpanNode changes the relationship between <p> and TextNode (from parent-child to grandparent-grandchild), which causes miscalculation for frame and offset.
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] from comment #19)
> In the simple testcase given in comment 17, two cases are given, the
> abnormal one on top and the normal one on bottom. In the abnormal case, the
> last child of <p> is SpanNode, not TextNode.

No, it's not. After the <span>, there is a text node containing exactly one whitespace character.

If you delete that text node, so the HTML is "</span></p>", you'll see that the test works.
Flags: needinfo?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> (In reply to Jeremy Chen [:jeremychen] from comment #19)
> > In the simple testcase given in comment 17, two cases are given, the
> > abnormal one on top and the normal one on bottom. In the abnormal case, the
> > last child of <p> is SpanNode, not TextNode.
> 
> No, it's not. After the <span>, there is a text node containing exactly one
> whitespace character.
> 
> If you delete that text node, so the HTML is "</span></p>", you'll see that
> the test works.

I made a new test as you said, please see https://jsfiddle.net/eqx37fps/12/ The position of end caret is still wrong.
Flags: needinfo?(roc)
(In reply to Jeremy Chen [:jeremychen] from comment #21)
> I made a new test as you said, please see https://jsfiddle.net/eqx37fps/12/
> The position of end caret is still wrong.

Oh, you're quite right. Yes, we'll need to recursively descend into the span in that case.
Flags: needinfo?(roc)
With this wip, the test in [1] can be passed, and the bug seems to be fixed on my local device. I've pushed this patch to try (see [2]). Some tests could be failed, so I'll keep working on fixing them.

[1] simple test https://jsfiddle.net/eqx37fps/12/
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5047382569aa
Attachment #8577853 - Attachment is obsolete: true
Attachment #8577855 - Attachment is obsolete: true
Attachment #8577853 - Flags: feedback?(roc)
Comment on attachment 8579928 [details] [diff] [review]
Part2: Recursive call GetFrameForNodeOffset if text node has no frame.

Hi roc,
This patch handle the issue which is text node without text frame. As you mentioned, If text node without frame, get previous or next node and call GetFrameForNodeOffset again.

Here is try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5eb1cfaeb4e
Attachment #8579928 - Flags: review?(roc)
After applying Part1 patch, which we let GetFrameForNodeOffset recursively descend into the leaf node, the test_scroll_selection_into_view would fail. It can be seen from the dom structure in [1], if we want to make the test functional, we need to ensure only scroll the element we want (in this test, the <span> node). Could I just remove the text node decended to the <span> node?

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/scroll_selection_into_view_window.html
Attachment #8581433 - Flags: feedback?(roc)
Attachment #8579928 - Attachment description: Recursive call GetFrameForNodeOffset if text node has no frame. → Part2: Recursive call GetFrameForNodeOffset if text node has no frame.
Looks like our part1 patch can fix a bug in bug414526, so we can reopen the test.
For testing scroll_into_view, we shall make sure only scroll the element we want. We can ensure this by removing the text inside the scrolled element.
Attachment #8581433 - Attachment is obsolete: true
Attachment #8581433 - Flags: feedback?(roc)
Move mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame, as comment 18 said.
Attachment #8582336 - Flags: review?(roc)
Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

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

It's not really clear to me why we need to do this. The test looks correct as it's currently written. Can you explain in more detail?
Attachment #8582261 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #34)
> Comment on attachment 8582261 [details] [diff] [review]
> Part4: Fix scroll_selection_into_view test to make its function remain.
> 
> Review of attachment 8582261 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It's not really clear to me why we need to do this. The test looks correct
> as it's currently written. Can you explain in more detail?

With the part1 patch, we let GetFrameForNodeOffset recursively descend into the leaf node. It can be seen from the dom structure in [1], if we want to make the test functional, we need to ensure only scroll the element we want (in this test, the <span> node, not the <text> node). So I just remove the text node, the decendant of the <span> node.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/base/tests/chrome/scroll_selection_into_view_window.html
Flags: needinfo?(roc)
Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

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

Yeah OK. This changes behavior of scroll_selection_into_view but I guess in some cases the new behavior will be better.
Attachment #8582261 - Flags: review+
try looks good except a known issue M12 for B2G ICS Emulator debug.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=438ce1eca7e7
Flags: needinfo?(roc)

Comment 39

4 years ago
This doesn't look safe to me. 'startFrame' might be destroyed after the flush:

nsIFrame* startFrame = FindFirstNodeWithFrame...
...
mPresShell->FlushPendingNotifications(Flush_Layout);
...
nsLayoutUtils::FindNearestCommonAncestorFrame(startFrame, ...
Comment on attachment 8582257 [details] [diff] [review]
Part1: (v4) Add recursive call in GetFrameForNodeOffset.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Selection carets may remain functional but look weird. (wrong position) 
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available
Attachment #8582257 - Flags: approval-mozilla-b2g37?
Comment on attachment 8579928 [details] [diff] [review]
Part2: Recursive call GetFrameForNodeOffset if text node has no frame.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Selection carets may remain functional but look weird. (wrong position) 
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available
Attachment #8579928 - Flags: approval-mozilla-b2g37?
Comment on attachment 8582336 [details] [diff] [review]
Part5: Move Flush_Layout ahead in SelectionCarets::UpdateSelectionCarets.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: not obvious, but this could prevent selection from potential bugs.
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available
Attachment #8582336 - Flags: approval-mozilla-b2g37?
Comment on attachment 8582260 [details] [diff] [review]
Part3: Fix a bug in bug414526 so we can reopen the test.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: none
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available
Attachment #8582260 - Flags: approval-mozilla-b2g37?
Comment on attachment 8582261 [details] [diff] [review]
Part4: Fix scroll_selection_into_view test to make its function remain.

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: none
Testing completed: on master
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: not available
Attachment #8582261 - Flags: approval-mozilla-b2g37?
Attachment #8579928 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8582257 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8582260 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8582261 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
Attachment #8582336 - Flags: approval-mozilla-b2g37? → approval-mozilla-b2g37+
This issue is NOT fixed on the latest Nightly Flame KK  3.0 and 2.2 builds.

Actual Results: The text selection carets appear above selected text or icon on the left side of a shared contact still.

The right side appears correctly.  See above screenshot.

Environmental Variables:
Device: Flame 3.0 KK (319MB) (Full Flash)
BuildID: 20150401010204
Gaia: 03164bd160809747e6a198e0dba1b7c3ee7789f5
Gecko: 18a8ea7c2c62
Gonk: b83fc73de7b64594cd74b33e498bf08332b5d87b
Version: 40.0a1 (3.0) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:40.0) Gecko/40.0 Firefox/40.0

Environmental Variables:
Device: Flame 2.2 KK (319MB) (Full Flash)
BuildID: 20150401002624
Gaia: 8b3086ad3963f1707e2bee9094baccafffe161c4
Gecko: 20b67213a047
Gonk: ebad7da532429a6f5efadc00bf6ad8a41288a429
Version: 37.0 (2.2) 
Firmware Version: v18D-1
User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0
QA Whiteboard: [QAnalyst-Triage+][MGSEI-Triage+][COM=Text Selection] → [QAnalyst-Triage?][MGSEI-Triage+][COM=Text Selection][failed-verification]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?][MGSEI-Triage+][COM=Text Selection][failed-verification] → [QAnalyst-Triage+][MGSEI-Triage+][COM=Text Selection][failed-verification]
Flags: needinfo?(ktucker)
Hey Jeremy, maybe you could help with what QA found in previous comment?
Flags: needinfo?(jeremychen)
Sorry, I can't reproduce it. I tested on the latest Nightly from pvt, and the left-caret looked good to me. See screenshot below.

Environmental Variables:
Gaia-Rev        4bb3a933bd805e8df1e11827cb247754c3565b0b
Gecko-Rev       https://hg.mozilla.org/mozilla-central/rev/37ddc5e2eb72
Build-ID        20150401180920
Version         40.0a1
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20150401.213521
FW-Date         Wed Apr  1 21:35:31 EDT 2015
Bootloader      L1TC10011880
Flags: needinfo?(jeremychen)
Flags: needinfo?(felash)

Comment 52

4 years ago
Did you file a follow-up bug on comment 39?  If not, please do so.
Flags: needinfo?(jeremychen)
(In reply to Mats Palmgren (:mats) from comment #52)
> Did you file a follow-up bug on comment 39?  If not, please do so.

Hi Mats, by applying Part5 patch, we move mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame and startFrame. Therefore, we should already avoid destroying rootFrame and startFrame. I'm not sure what follow-up bug you want me to file. Could you provide more information?
Flags: needinfo?(jeremychen)
The placement of the caret does not look especially wrong. However, I can't really move the carets when there is an attachment in a message... It works well when there is only text though.
Flags: needinfo?(felash)
Hi Julien, with an attachment in a text message, moving carets in the text part works find. I think
you mean moving caret on the attachment and its file name, right? Please see the video below.

According to commment 8, the file name should not be selectable in the first place. So, I'm not sure if we should file another bug to deal with this.
I tested with a message that is like this:

  ------------
  |Attachment|
  ------------
  some text

Then when I press "select text", it's very difficult to move the left caret. At least for me. Maybe the issue is me :)
(In reply to Julien Wajsberg [:julienw] (PTO April 6th) from comment #57)
> I tested with a message that is like this:
> 
>   ------------
>   |Attachment|
>   ------------
>   some text
> 
> Then when I press "select text", it's very difficult to move the left caret.
> At least for me. Maybe the issue is me :)

I dug a little deeper and found you're right. I've filed another bug (Bug 1152263). Thanks for reporting this. :)
Thanks to you !

Comment 60

4 years ago
(In reply to Jeremy Chen [:jeremychen] from comment #53)
> (In reply to Mats Palmgren (:mats) from comment #52)
> > Did you file a follow-up bug on comment 39?  If not, please do so.
> 
> Hi Mats, by applying Part5 patch, we move
> mPresShell->FlushPendingNotifications(Flush_Layout) before we get rootFrame
> and startFrame. Therefore, we should already avoid destroying rootFrame and
> startFrame.

OK, looks good so far, but I think we need to move it all they way to the start
of the method, since mPresShell and |this| might be destroyed after the flush.
I filed bug 1152432.
You need to log in before you can comment on or make changes to this bug.