1243354 - crash in java.lang.ClassCastException: android.support.design.widget.TextInputLayout cannot be cast to android.widget.EditText at org.mozilla.gecko.prompts.PromptInput$EditInput.getValue(PromptInput.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Kartikaya Gupta (email:[e-mail])]

This bug was filed from the Socorro interface and is 
report bp-3f7ce6c8-7c22-4c82-a9cc-a95e82160127.
=============================================================

Trying to go to a page with http auth dialog.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Guessing it's a regression from bug 1227325.

Comment 2

[:Margaret Leibovic]

Why did we fix bug 1227325? What's the value we get there? Should we just back out that change?

Comment 3

[Andrzej Hunt :ahunt]

(In reply to :Margaret Leibovic from comment #2)
> Why did we fix bug 1227325? What's the value we get there? Should we just
> back out that change?

The main reason was to avoid using our custom FloatingHintEditText since the design support library provides an equivalent. Our custom widget also behaves differently from the design support library version (i.e. differently from the input widgets that any other app uses).

Fixing this needs some cleanup of PromptInput (it does some complicated things around storing the View for subclasses in PromptInput, with the subclasses then upcasting their underlying View to store it in PromptInput, and then downcasting when reading data - which is where this bug happened) - I'm not sure how backportable this patch will be, so we might want to backout on aurora?

Comment 4

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1243354 - Part 1: Correctly handle TextInputLayout in PromptInput r=mcomella

Review commit: https://reviewboard.mozilla.org/r/32845/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32845/

Comment 5

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #3)
> Fixing this needs some cleanup of PromptInput (it does some complicated
> things around storing the View for subclasses in PromptInput, with the
> subclasses then upcasting their underlying View to store it in PromptInput,
> and then downcasting when reading data - which is where this bug happened) -
> I'm not sure how backportable this patch will be, so we might want to
> backout on aurora?

The fix for the crash is actually quite simple (see Part 1 on reviewboard) - and that fix is probably backportable. However PromptInput has a lot of unnecessary type-casting, and other unsafe operations, which I'm going to address in Part 2 cleanup patch.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8713877 [details]
MozReview Request: Bug 1243354 - Part 1: Correctly handle TextInputLayout in PromptInput r=mcomella

https://reviewboard.mozilla.org/r/32845/#review29705

Is there any way we can tell if there's any other of these sneaky references to the view hanging around? I'm afraid of us bumping into another crash.

::: mobile/android/base/java/org/mozilla/gecko/prompts/PromptInput.java:112
(Diff revision 1)
> -            EditText input = (EditText) super.getView(context);
> +            TextInputLayout inputLayout = (TextInputLayout) super.getView(context);
> +            EditText input = inputLayout.getEditText();

nit: `final` on most of the changed lines

Comment 7

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/32845/#review29705

I'm cleaning that up in the (not yet finished) Part 2 (all of PromptInput, and the other derived classes that live elsewhere) - I want Part 1 to be something simple that's backportable!

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8713877 [details]
MozReview Request: Bug 1243354 - Part 1: Correctly handle TextInputLayout in PromptInput r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32845/diff/1-2/

Comment 9

[Andrzej Hunt :ahunt]

Comment on attachment 8713877 [details]
MozReview Request: Bug 1243354 - Part 1: Correctly handle TextInputLayout in PromptInput r=mcomella

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32845/diff/2-3/

Comment 10

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: WIP: Bug 1243354 - Sanitise View creation and management in PromptInput and subclasses

Review commit: https://reviewboard.mozilla.org/r/32847/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32847/

Comment 11

[Andrzej Hunt :ahunt]

Comment on attachment 8713887 [details]
MozReview Request: WIP: Bug 1243354 - Sanitise View creation and management in PromptInput and subclasses

This is a first attempt to simplify what we're doing in PromptInput.

One of the bigger changes is shifting the View creation for each input type from getView into the constructor. I don't understand why we do that, more importantly the current implementation relies on getView() not being called more than once. (However we could also avoid that by checking whether we've already constructed the View in getView.)

Comment 12

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #11)
> Comment on attachment 8713887 [details]
> MozReview Request: WIP: Bug 1243354 - Sanitise View creation and management
> in PromptInput and subclasses
> 
> This is a first attempt to simplify what we're doing in PromptInput.
> 
> One of the bigger changes is shifting the View creation for each input type
> from getView into the constructor. I don't understand why we do that, more
> importantly the current implementation relies on getView() not being called
> more than once. (However we could also avoid that by checking whether we've
> already constructed the View in getView.)

Note primarily for myself: we should look into adding tests for these prompts. We have testPromptGridInput already, which tests just the creation of that prompt from JS, but we should add tests checking that we can:

(1) Create all input fields from JS
(2) Show a login prompt for HTTP Auth pages (that might be trickier to test?)
(3) Show the correct prompt (with data pre-entered if supplied) when an input field is selected in an actual page

Comment 13

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/f306359ac4f4eccc7eb81b9c8ec80597bad94e5d
Bug 1243354 - Part 1: Correctly handle TextInputLayout in PromptInput r=mcomella

Comment 14

[Andrzej Hunt :ahunt]

The crashfix is now on fx-team, but I'm still working on tidying the surrounding code up - hence I'd like to leave the bug open for now.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Andrzej Hunt :ahunt from comment #14)
> The crashfix is now on fx-team, but I'm still working on tidying the
> surrounding code up - hence I'd like to leave the bug open for now.

I would prefer if the tidying was put into a separate bug – is there any reason not to?

Comment 16

[Andrzej Hunt :ahunt]

(In reply to Michael Comella (:mcomella) from comment #15)
> (In reply to Andrzej Hunt :ahunt from comment #14)
> > The crashfix is now on fx-team, but I'm still working on tidying the
> > surrounding code up - hence I'd like to leave the bug open for now.
> 
> I would prefer if the tidying was put into a separate bug – is there any
> reason not to?

Filed as Bug 1244928, now removing leave-open again.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f306359ac4f4

Comment 19

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8713887 [details]
MozReview Request: WIP: Bug 1243354 - Sanitise View creation and management in PromptInput and subclasses

Clearing f?.

Andrzej, can you push this changeset to the other bug and flag me for feedback again (if you're actively working on it, that is)?

Comment 20

[Andrzej Hunt :ahunt]

(In reply to Michael Comella (:mcomella) from comment #19)
> Comment on attachment 8713887 [details]
> MozReview Request: WIP: Bug 1243354 - Sanitise View creation and management
> in PromptInput and subclasses
> 
> Clearing f?.
> 
> Andrzej, can you push this changeset to the other bug and flag me for
> feedback again (if you're actively working on it, that is)?

Clearing NI: I've pushed to code into the new bug. (I doubt it's a good idea to push this soon in case our refactoring causes fallout.)