477700 - [FIX]Can't type into text input when input is in a subframe that has its presentation reconstructed via layout flush on parent

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Martijn Wargers (dead)]

Attachment: frame page with input needed for testcase

See upcoming testcase, you should be able to type some text in the text input, but that's not possible in current trunk build.

This regressed between 2009-01-29 and 2009-01-30:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2009-01-29+04%3A00%3A00&enddate=2009-01-30+06%3A00%3A00
I think a regression from bug 335615.

I'm also seeing this warning in my debug build:
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x805303E8: file c:/mozill
a-build-1.3/src/layout/forms/nsTextControlFrame.cpp, line 1456
..which seems related to that bug.

Note that the iframe needs to be from another domain to let the bug occur, it seems.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: testcase

Comment 3

[Martijn Wargers (dead)]

Attachment: testcase

Comment 4

[Martijn Wargers (dead)]

Attachment: testcase

Sorry for the testcase spam, but I tried to get a working testcase that only used bugzilla (the new bugzilla setup might have allowed that), but that didn't work. Now I've put the iframe page on my googlepages site.

Comment 5

[Boris Zbarsky [:bzbarsky]]

The error is, unsurprisingly, NS_ERROR_DOM_SECURITY_ERR.

It happens because we run the script runner from the layout (or more precisely frame reconstruct) flush triggered by the FlushPendingNotifications.  At this point, the outer document's JS is on the stack, so when we try to set up a range inside the editor (in nsTextEditRules::Init) it fails.

This is easy to fix, but I want to try to understand why this wasn't a problem before.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Oh, and the height change is not needed to reproduce the bug.

Comment 7

[Boris Zbarsky [:bzbarsky]]

OK, I have no clue why it used to "work", but it didn't "work" very well (e.g. undo/redo didn't work, etc).  I guess we used to sorta do InitEditor but not other parts of editor init...

The right fix is just to make sure to push a null JSContext on the stack, but that should happen in general around script runner processing?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed fix

Comment 9

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

In this case nsCxPusher should work too.
nsRange checks principals, and context from mContent should give the right
principal.

But, if you really want to use null context, please make some helper
class to automatically push/pop (or modify nsCxPusher to support null cx).

Comment 10

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

Comment on attachment 361533 [details] [diff] [review]
Proposed fix

Either way, but I really don't like manual context push.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Hmm.  nsCxPusher does this thing where it calls ScriptEvaluated.  I'm not sure that's desirable here.  I'm happy to add a class that'll do an automatic null push, or even an automatic push of whatever context, I guess.  Let me know what your thoughts on the ScriptEvaluated business are?

Comment 12

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

No ScriptEvaluated when null context is used.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Yes, but nsCxPusher won't let you push a null context.  In fact, an attempt to do so is a no-op.

Do you want me to fix it so that it allows usefully pushing null contexts, then?  Or so that it allows pushing from an element without calling ScriptEvaluated?

I suppose as long as I'm here I should fix the fact that it doesn't deal with Push() failure too, eh?  :(

Comment 14

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

(In reply to comment #13)
> Yes, but nsCxPusher won't let you push a null context.
Which is why I said that you could modify nsCxPusher to support null context.

> Do you want me to fix it so that it allows usefully pushing null contexts,
> then?  Or so that it allows pushing from an element without calling
> ScriptEvaluated?
Whatever is easier. Though, since null context is anyway needed elsewhere, might
be useful to have PushNull() method in nsCXPusher. When that is used,
ScriptEvaluated shouldn't be called.

I think Push(null) should always fail.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Updated to comments

The test for bug 287446 works on trunk with just the new null push, but I did verify that at the time that bug's patch landed it would have caught it...  Maybe we don't need the other two null pushes at all anymore, but I figured I wouldn't risk it.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Diff -w version for review only

Comment 17

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

Comment on attachment 362517 [details] [diff] [review]
Diff -w version for review only

Check the return value of pusher.PushNull() and return early if it failed?

Comment 18

[Boris Zbarsky [:bzbarsky]]

I'm not sure that's desirable, actually.  The worst-case scenario is that it fails and there's content script on the stack and then the editor operation won't work.  If I bail the editor operation _certainly_ won't work...

Since I fully expect it to only fail on OOM, by the way, I expect it to become a void method in the future.

Given all that, I'd rather not spend the time now on trying to figure out a "safe" way to return early here...  If you feel strongly about this I can try, of course.  Let me know?

Comment 19

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

Ok, no need for the return value check.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/742c2f73fe44

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: Additional patch needed to fix orange

The extension manager tests do XHR in xpcshell, where there is no script context in sight.  We were returning false from the event target version of Push() as a result (because we had a null JSContext*), which caused the events involved to never get dispatched and the test to time out (it was waiting on the events).

This patch fixes the orange.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Pushed the additional patch as http://hg.mozilla.org/mozilla-central/rev/0a901e2dd044