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.
02-15 2302 ok
02-16 nightly broken
From the patch in bug 392785 it looks like the last parameter needs to be changed to false in viewSource.js:
There may be other places to change as well e.g.:
(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:
> There may be other places to change as well e.g.:
And a couple more:
the patch coming soon.
Created attachment 304575 [details] [diff] [review]
I think that the cause of this bug is behavior changing of layout, so, this should be reviewed by layout reviewer.
Actually, shouldn't synchronous ScrollSelectionIntoView be flushing layout?
Created attachment 305038 [details] [diff] [review]
how about this?
this patch also includes backing out patch of the additional patch in bug 392785.
Comment on 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.
(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?
If I change
return mDomSelections[index]->ScrollIntoView(aRegion, 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.
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?
(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
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.
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.
You said "nsPresShell pointer should not be held." ScrollSelectionIntoView is called from nsPresShell with synchronous, is it ok? (strong reference saves the presShell?)
> 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?
(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
Yes. But I abort the checking when the stack item is js.
I'm worried that ScrollSelectionIntoView breaks the caller presShell by the flushing.
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.
Created attachment 305840 [details] [diff] [review]
Do I need to add the notice comments to all callers? (and also to all ancestor callers??)
+ // 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.
Created attachment 305982 [details] [diff] [review]
Thank you roc. The ELM part is using synchronous now.
+ // After FixContextMenuEvent(), the pending reflow might be flushed and
+ // 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.
Created attachment 305984 [details] [diff] [review]
updating the comments.
Comment on attachment 305984 [details] [diff] [review]
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.
Moving bugs that aren't beta 4 blockers to target final release.
Comment on attachment 305984 [details] [diff] [review]
*** 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
# 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??
I cannot reproduce the failure and tinderbox already succeeded the test before backed out. Therefore, I rechecked-in the patch.