Closed Bug 430351 Opened 12 years ago Closed 12 years ago

unable to fill certain flash forms with recent camino nightly builds

Categories

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

PowerPC
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla1.9

People

(Reporter: tpennycook, Assigned: peterv)

References

()

Details

Attachments

(1 file, 2 obsolete files)

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
Confirmed, and surprisingly I can't reproduce this in today's Minefield build. We should get a regression range on this.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Severity: normal → major
Flags: camino2.0a1+
Target Milestone: --- → Camino2.0
4/15 trunk works
4/16 trunk fails
Flags: blocking1.9?
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
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.
Blocks: 406596
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.
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.
Flags: camino2.0a1+
Product: Camino → Core
QA Contact: plugins → plugins
Target Milestone: Camino2.0 → ---
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.
Flags: in-testsuite?
The tests for this would ideally test the output of IsHTMLFocusable through all its codepahts...
Assignee: nobody → peterv
Attached patch v1 (obsolete) — Splinter Review
Attachment #317138 - Flags: superreview?
Attachment #317138 - Flags: review?(jonas)
Attachment #317138 - Flags: superreview? → superreview?(bzbarsky)
Rereading the patch in bug 406596, the other codepaths should be ok. Will write tests.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9
Version: unspecified → Trunk
Flags: blocking1.9? → blocking1.9+
Duplicate of this bug: 429771
Comment on attachment 317138 [details] [diff] [review]
v1

Looks reasonable.  Thanks for the tests in advance!
Attachment #317138 - Flags: superreview?(bzbarsky) → superreview+
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 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.
Attachment #317138 - Flags: review?(jonas) → review+
(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.
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.
(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.
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.
(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).
What are we waiting for? Seems like we have a fix in hand but no activity here in 4 days.
Attached patch Mochitest (obsolete) — Splinter Review
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 on attachment 317138 [details] [diff] [review]
v1

Low-risk fix for a regression, have mochitest.
Attachment #317138 - Flags: approval1.9?
(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 on attachment 317138 [details] [diff] [review]
v1

a=beltzner, please land with test
Attachment #317138 - Flags: approval1.9? → approval1.9+
This is a single patch with the test and a default value.
Attachment #317138 - Attachment is obsolete: true
Attachment #318387 - Attachment is obsolete: true
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.
Attachment #318509 - Flags: review+
checked in
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Flags: in-testsuite? → in-testsuite+
Duplicate of this bug: 434111
You need to log in before you can comment on or make changes to this bug.