430351 - unable to fill certain flash forms with recent camino nightly builds

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P1)

Description

[Tim Pennycook]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9pre) Gecko/2008042100 Camino/2.0a1pre (like Firefox/3.0pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9pre) Gecko/2008042100 Camino/2.0a1pre (like Firefox/3.0pre)

After updating Camino from a nightly-build a couple weeks old I found the flash forms at www.beatport.com do not allow one to enter text. Works fine in other browsers, including Camino 1.6.  Running Shockwave Flash 9.0 r115. New here, so hope this is how to do things (and yes I did look to see if this would be a dupe).

Reproducible: Always

Steps to Reproduce:
1.Go to www.beatport.com
2.try to enter text in any of the text fields, like in the search field upper left corner
Actual Results:  
no text appears on attempting to enter text in the flash form field 

Expected Results:  
text should appear and be enterable

Comment 1

[Stuart Morgan]

Confirmed, and surprisingly I can't reproduce this in today's Minefield build. We should get a regression range on this.

Comment 2

[Stuart Morgan]

4/15 trunk works
4/16 trunk fails

Comment 3

[philippe (part-time)]

Surprisingly, with my home made build (10.5 sdk/10.5.2), it works fine.

With an official build, I can paste a text string, but only using the Flash context menu. I can then select that string, but not edit it.

Flash 9 r124

Comment 4

[Stuart Morgan]

Backing out bug 406596 (the non-test portions) fixes this in my build.

Someone should check to see if the same is true of bug 429771.

Comment 5

[Stuart Morgan]

http://www.oman3d.com/tutorials/flash/password_bc/ (from bug 429771) is also broken in Camino.

Comment 6

[Stuart Morgan]

This is pure speculation, but the fact that it varies by compilation makes me wonder if the cause is:

+  virtual PRBool IsFocusable(PRInt32 *aTabIndex = nsnull)
+  {
+    PRBool isFocusable;
+    IsHTMLFocusable(&isFocusable, aTabIndex);
+    return isFocusable;
+  }

isFocusable is uninitialized, and the very first code path out of isHTMLFocusable in at least most cases is:

+nsHTMLAnchorElement::IsHTMLFocusable(PRBool *aIsFocusable, PRInt32 *aTabIndex)
 {
-  if (!nsGenericHTMLElement::IsFocusable(aTabIndex)) {
-    return PR_FALSE;
+  if (nsGenericHTMLElement::IsHTMLFocusable(aIsFocusable, aTabIndex)) {
+    return PR_TRUE;
+  }
+

which doesn't set *aIsFocusable.

Comment 7

[Stuart Morgan]

Sorry, I missesd that aIsFocusable is passed through, so it may be set lower down. I do wonder if there is a code path where it's never set though.

Comment 8

[Boris Zbarsky [:bzbarsky]]

So looking at the patch for bug 406596:

nsHTMLImageElement::IsHTMLFocusable in the imagemap case sets *aIsFocusable only to have it clobbered later in the function.

nsHTMLSharedObjectElement::IsHTMLFocusable never sets *aIsFocusable in the "is a plug-in" codepath, and comment 6 is spot-on about what happens in that case.

The latter is likely what causes this bug: we end up doing random things based on what memory values happen to be on the stack.  I think this should block 1.9.

Comment 9

[Boris Zbarsky [:bzbarsky]]

The tests for this would ideally test the output of IsHTMLFocusable through all its codepahts...

Comment 10

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 11

[Peter Van der Beken [:peterv]]

Rereading the patch in bug 406596, the other codepaths should be ok. Will write tests.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 317138 [details] [diff] [review]
v1

Looks reasonable.  Thanks for the tests in advance!

Comment 14

[Stuart Morgan]

Comment on attachment 317138 [details] [diff] [review]
v1

Perhaps it would be a good idea to also change IsFocusable to initialize isFocusable to PR_FALSE so that if there is ever another bad code path, the results will be deterministic, and thus hopefully easier to find?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 317138 [details] [diff] [review]
v1

Should you make nsHTMLImageElement return PR_TRUE instead. The return value seems to be an indicator to subclasses "I've set aIsFocusable".

Guess it doesn't matter since no-one is subclassing that images anyway.

And yes, setting giving a default value seems like a good idea. Though possibly something that should wait until after the initial release so that we don't set it to the wrong value.


And please do write tests.

Comment 16

[Stuart Morgan]

(In reply to comment #15)
> Though possibly something that should wait until after the initial release so
> that we don't set it to the wrong value.

I'm not sure how PR_FALSE could be more wrong than undefined (and therefore theoretically different in a minor update that would otherwise have no effect--or even in the last build before 3.0 ships), but since all the current code paths have now been audited it's more a future-proofing suggestion than anything else, so it probably doesn't matter if it goes in later.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Well, clearly it would often default to PR_TRUE since if it defaulted to PR_FALSE <object>s couldn't be focused at all before this patch.

Comment 18

[Stuart Morgan]

(In reply to comment #17)
> if it defaulted to PR_FALSE <object>s couldn't be focused at all before this
> patch.
 
Right, which was precisely the behavior that most Camino builds and at least some debug Firefox builds on the Mac had, so clearly it's not too reliably turning out to be PR_TRUE. My thought was just that "<foo> elements can't be focused in any build of any product" can be caught and fixed immediately, instead of potentially torpedoing us down the line with sudden inexplicable failures in a later build (or worse, intermittent failures in the shipped product) as might well have happened to Firefox if it hadn't turned out to be PR_FALSE in Camino and caught that way.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I agree in general, i'm just slightly worried about taking that route this close to shipping.

Though the best strategy is probably to audit all code paths to make sure that we always set the right value, and at that point it shouldn't matter what we set the initial value to.

So if Peter feels secure enough that all code has been checked i'm fine with setting the default to PR_FALSE.

Comment 20

[Peter Van der Beken [:peterv]]

(In reply to comment #15)
> (From update of attachment 317138 [details] [diff] [review])
> Should you make nsHTMLImageElement return PR_TRUE instead. The return value
> seems to be an indicator to subclasses "I've set aIsFocusable".

Actually, the return value is an indicator of whether the subclass can override the value, for example nsGenericHTMLElement checks for designmode and then overrides the focusability of elements (makes it to false).

Comment 21

[Josh Aas]

What are we waiting for? Seems like we have a fix in hand but no activity here in 4 days.

Comment 22

[Peter Van der Beken [:peterv]]

Attachment: Mochitest

Here are the mochitests, this tests all the codepaths in IsHTMLFocusable, except 2: frame with a zombie document and object elements with a plugin (I don't think we have a plugin in the testing environment that I could use). Note that this just tests the existing implementations of IsHTMLFocusable, I don't think they always do the right thing but I'll file follow up bugs on that and we can adjust the tests as we fix them.

Comment 23

[Peter Van der Beken [:peterv]]

Comment on attachment 317138 [details] [diff] [review]
v1

Low-risk fix for a regression, have mochitest.

Comment 24

[Peter Van der Beken [:peterv]]

(In reply to comment #21)
> What are we waiting for? Seems like we have a fix in hand but no activity here
> in 4 days.

It took a while to write the mochitests. There was plenty of activity, just none in the bug.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 317138 [details] [diff] [review]
v1

a=beltzner, please land with test

Comment 26

[Josh Aas]

Attachment: v1 + default value + test

This is a single patch with the test and a default value.

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 318509 [details] [diff] [review]
v1 + default value + test

Josh requested that we land this ASAP to make it easier to deal with the general mess we have with keys. So lets land this for now and if we want another default value fix that in a followup patch.

Comment 28

[Josh Aas]

checked in