Go to line in View Source broken (NS_ERROR_UNEXPECTED at nsISelectionController.scrollSelectionIntoView)

RESOLVED FIXED in mozilla1.9beta4

Status

()

Core
Layout
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Steffen Wilberg, Assigned: masayuki)

Tracking

({regression})

Trunk
mozilla1.9beta4
regression
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
When clicking on errors in the Error Console, View Source displays the file, but doesn't scroll down to the line of the error. If you scroll down manually, you'll find the line selected though. Affects Firebug as well.

When using View Source -> menu Edit -> Go to Line (Ctrl+L), this error is displayed in the Error Console:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsISelectionController.scrollSelectionIntoView]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://global/content/viewSource.js :: goToLine :: line 372"  data: no]

This is with today's Minefield nightly on Windows.
Works with Firefox 2.
Flags: blocking1.9?

Comment 1

9 years ago
Regression range 

02-15 2302 ok
02-16 nightly broken

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1203145320&maxdate=1203176399

Blocks: 392785

Comment 2

9 years ago
From the patch in bug 392785 it looks like the last parameter needs to be changed to false in viewSource.js:
http://lxr.mozilla.org/mozilla/source/toolkit/components/viewsource/content/viewSource.js#406

There may be other places to change as well e.g.:
http://lxr.mozilla.org/mozilla/source/toolkit/components/viewsource/content/viewPartialSource.js#309

(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> From the patch in bug 392785 it looks like the last parameter needs to be
> changed to false in viewSource.js:
> http://lxr.mozilla.org/mozilla/source/toolkit/components/viewsource/content/viewSource.js#406
> 
> There may be other places to change as well e.g.:
> http://lxr.mozilla.org/mozilla/source/toolkit/components/viewsource/content/viewPartialSource.js#309

Exactly.
(Reporter)

Comment 4

9 years ago
And a couple more:
http://mxr.mozilla.org/seamonkey/search?string=scrollSelectionIntoView&find=.js&findi=&filter=&tree=seamonkey
(Assignee)

Comment 5

9 years ago
the patch coming soon.
Assignee: nobody → masayuki
OS: Windows Vista → All
Hardware: PC → All
Target Milestone: --- → mozilla1.9beta4
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 6

9 years ago
Created attachment 304575 [details] [diff] [review]
Patch v1.0

I think that the cause of this bug is behavior changing of layout, so, this should be reviewed by layout reviewer.
Attachment #304575 - Flags: superreview?(roc)
Attachment #304575 - Flags: review?(roc)
Actually, shouldn't synchronous ScrollSelectionIntoView be flushing layout?
(Assignee)

Comment 8

9 years ago
Created attachment 305038 [details] [diff] [review]
Patch v2.0

how about this?

this patch also includes backing out patch of the additional patch in bug 392785.
Attachment #304575 - Attachment is obsolete: true
Attachment #305038 - Flags: superreview?(roc)
Attachment #305038 - Flags: review?(roc)
Attachment #304575 - Flags: superreview?(roc)
Attachment #304575 - Flags: review?(roc)
Comment on attachment 305038 [details] [diff] [review]
Patch v2.0

Looks good, but have you checked that all ScrollSelectionIntoView callers are safe for flushing? They should not be holding nsIFrame* pointers, for example.
Attachment #305038 - Flags: superreview?(roc)
Attachment #305038 - Flags: superreview+
Attachment #305038 - Flags: review?(roc)
Attachment #305038 - Flags: review+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> (From update of attachment 305038 [details] [diff] [review])
> Looks good, but have you checked that all ScrollSelectionIntoView callers are
> safe for flushing? They should not be holding nsIFrame* pointers, for example.

I'm not sure. Is there a good way for checking it?
(Assignee)

Comment 11

9 years ago
If I change

   return mDomSelections[index]->ScrollIntoView(aRegion, aIsSynchronous,
-                                               PR_FALSE);
+                                               PR_TRUE);

to

+                                               aIsSynchronous);

The users can choose the flush safe or not. And the risk is lower. How about this?
That's safer. You still should have a look at the callers who are passing true for aIsSynchronous. They should not be holding frame pointers across the ScrollIntoView call, and they should not be holding nsPresContext* or nsPresShell* pointers either.
(Assignee)

Comment 13

9 years ago
It sounds like my latest patch is very risky. Because I cannot check all callers of the ScrollSelectionIntoView callers. I think that for API compatibility, we should flush layout if aIsSynchronus is true, however, should I change the param of all callers in Gecko changing to PR_FALSE? There are 13 points in .cpp file. (I think that the risk is too low in .js files (5 points) for most cases.)
See bug 375436 BTW.

> That's safer.

Actually it makes no difference; if aIsSynchronous is false then aDoFlush is ignored.

Changing all callers to async isn't actually safe since they might be relying on the scroll being synchronous, changing that could break their later operations. So we need to check the call sites no matter what.

I checked nsTypeaheadFind (both versions) and nsMsgCompose and those callers are safe. All JS callers must be safe already. You should be able to audit the rest.

Boris, in bug 375436 you made ScrollSelectionIntoView not flush for synchronous scrolls. Do you remember any specific caller/situation that could not handle a flush there?
(Assignee)

Comment 15

9 years ago
(In reply to comment #14)
> I checked nsTypeaheadFind (both versions) and nsMsgCompose and those callers
> are safe. All JS callers must be safe already. You should be able to audit the
> rest.

Note that the backing out positions are not all. I didn't change in some points PR_TRUE to PR_FALSE if they don't change selections before calling. (But we might be able to think that the current caller must not have any pending reflows excepting the new reflows of bug 392785.)
> Do you remember any specific caller/situation that could not handle a
> flush there?

At least text control frame, iirc.  But basically, I seem to recall a bunch of callers, with a very branchy callstack so that I couldn't audit them in a reasonable amount of time...
And to be honest, I'm not quite sure what the use cases for a sync scroll here are...
Thanks. Yeah, the scrolls probably don't need to be synchronous, but code might depend on them being synchronous.

Okay then, the JS callers should stay synchronous and flushing if they want to be. nsTypeheadFind (both versions) and nsMsgCompose look safe to be synchronous. The others we need to audit and either verify that they're safe or convert to asynchronous.
(Assignee)

Comment 19

9 years ago
ok, I'm checking nsWebBrowserFind.cpp and nsHyperTextAccessible.cpp.
To be honest, in my ideal world the scroll would always be async but a flush would flush out pending scrolls or something...
With compositor, the frame tree will update synchronously but the actual scrolling will be deferred until the new window update.
(Assignee)

Comment 22

9 years ago
roc:

You said "nsPresShell pointer should not be held." ScrollSelectionIntoView is called from nsPresShell with synchronous, is it ok? (strong reference saves the presShell?)


safe:
nsWebBrowserFind::SetSelectionAndScroll
nsEditor::ScrollSelectionIntoView

unsafe:
nsEventListenerManager::PrepareToUseCaretPosition

unknown:
nsHyperTextAccessible::SetSelectionRange
> strong reference saves the presShell?

I'd be somewhat doubtful.

I assume for the "safe" cases you checked all possible callstacks to make sure that none of the possible callers are holding any presshell or frame references?
(Assignee)

Comment 24

9 years ago
(In reply to comment #23)
> I assume for the "safe" cases you checked all possible callstacks to make sure
> that none of the possible callers are holding any presshell or frame
> references?

Yes. But I abort the checking when the stack item is js.

I'm worried that ScrollSelectionIntoView breaks the caller presShell by the flushing.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
PresShell::PageMove and PresShell::CompleteMoveInner look safe. The ultimate callers are nsEditorCommands and nsGlobalWindowCommands, which look safe, and each call along the path is at the end of the caller's method. We'd need some comments along those paths explaining that a flush might have occurred and the presshell dead (although not deleted, since the selection caller holds a strong ref to nsISelectionController which is the presshell). So nsPresShell should be OK.
We need need need Taras-annotations and analysis for this stuff.
(Assignee)

Comment 27

9 years ago
Created attachment 305840 [details] [diff] [review]
Patch v3.0

Do I need to add the notice comments to all callers? (and also to all ancestor callers??)
Attachment #305038 - Attachment is obsolete: true
Attachment #305840 - Flags: superreview?(roc)
Attachment #305840 - Flags: review?(roc)
+    // This SecrollSelectionIntoView should not call as synchronous scrolling,
+    // because it might flush the layout reflow.
+    // It might be dangerous at aPresShell.
     rv = selCon->ScrollSelectionIntoView(nsISelectionController::SELECTION_NORMAL,
-        nsISelectionController::SELECTION_FOCUS_REGION, PR_TRUE);
+        nsISelectionController::SELECTION_FOCUS_REGION, PR_FALSE);

This is probably not a good idea. After this, the code is messing around with caret coordinates and those might be wrong if the scroll has not happened yet. We should make this path safe for synchronous scrolling. This function actually looks OK, and its caller nsEventListenerManager::FixContextMenuEvent also looks OK, and its caller nsEventListenerManager::HandleEvent should be OK since event handling can do pretty much anything already. (But these functions should be documented as potentially flushing.)
It would be good if all functions that can flush said so in their documentation, yes.
(Assignee)

Comment 30

9 years ago
Created attachment 305982 [details] [diff] [review]
Patch v4.0

Thank you roc. The ELM part is using synchronous now.
Attachment #305840 - Attachment is obsolete: true
Attachment #305982 - Flags: superreview?(roc)
Attachment #305982 - Flags: review?(roc)
Attachment #305840 - Flags: superreview?(roc)
Attachment #305840 - Flags: review?(roc)
+  // After FixContextMenuEvent(), the pending reflow might be flushed and

"pending notifications"

+  // PresShell/PresContext/Frames will be dead.

"may be dead"

Actually it would be more concise at all these sites to just say
  // Beware! This may flush notifications via synchronous ScrollSelectionIntoView.

Sorry! But if you fix that we should be good.
(Assignee)

Comment 32

9 years ago
Created attachment 305984 [details] [diff] [review]
Patch v4.1

updating the comments.
Attachment #305982 - Attachment is obsolete: true
Attachment #305984 - Flags: superreview?(roc)
Attachment #305984 - Flags: review?(roc)
Attachment #305982 - Flags: superreview?(roc)
Attachment #305982 - Flags: review?(roc)
Attachment #305984 - Flags: superreview?(roc)
Attachment #305984 - Flags: superreview+
Attachment #305984 - Flags: review?(roc)
Attachment #305984 - Flags: review+
(Assignee)

Comment 33

9 years ago
Comment on attachment 305984 [details] [diff] [review]
Patch v4.1

This patch is pretty risky. But this patch changes the API (ScrollSelectionIntoView) behavior. So, we should land this to b4 and many testers (including addon developers) should test this.
Attachment #305984 - Flags: approval1.9b4?
Moving bugs that aren't beta 4 blockers to target final release.
Target Milestone: mozilla1.9beta4 → mozilla1.9

Updated

9 years ago
Blocks: 419636
Comment on attachment 305984 [details] [diff] [review]
Patch v4.1

a1.9b4=beltzner
Attachment #305984 - Flags: approval1.9b4? → approval1.9b4+
(Assignee)

Comment 36

9 years ago
checked-in.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 → mozilla1.9beta4
(Assignee)

Comment 37

9 years ago
backed out.

*** 11859 ERROR FAIL | Should not be able to navigate popup's popup by submitting form. | got 1, expected 2 | /tests/docshell/test/navigation/test_not-opener.html

*** 11860 ERROR FAIL | Should not be able to navigate popup's popup by targeted hyperlink. | got 1, expected 2 | /tests/docshell/test/navigation/test_not-opener.html
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 38

9 years ago
mmmm....
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1204204620.1204207474.9549.gz
# find "/tests/docshell/test/navigation/test_not-opener.html"

> *** 11857 INFO Running /tests/docshell/test/navigation/test_not-opener.html...
> *** 11858 INFO PASS | Should not be able to navigate popup's popup by calling window.open.
> *** 11859 INFO PASS | Should not be able to navigate popup's popup by submitting form.
> *** 11860 INFO PASS | Should not be able to navigate popup's popup by targeted hyperlink.

After the failure, the next test time didn't orange. Isn't it a my bug??
(Assignee)

Comment 39

9 years ago
I cannot reproduce the failure and tinderbox already succeeded the test before backed out. Therefore, I rechecked-in the patch.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.