803112 - [AccessFu] AccessFu completely broken starting in the 2012-10-18 nightly build

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Marco Zehe (:MarcoZ)]

AccessFu is completely broken, no longer shows any web site content. The only thing that works is the native UI.

Regression range:
hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dac5700acf8b&tochange=cb573b9307e5

Investigation so far has revealed that bug 801671 is not the culprit. This was the only AccessFu bug that landed in the regression range.
Also, looking at the logcat output, I only saw one getBrowser() returns NULL error message immediately after the AccessFu Ready entry. No further error messages from AccessFu.

I suspect it might have something to d with the services refactor, bug 801225, which falls into this range.

Comment 1

[Eitan Isaacson [:eeejay]]

Seems to still work on GB, but not on JB.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Handle AccessFu startup when there is no current browser yet.

Comment 3

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 672930 [details] [diff] [review]
Handle AccessFu startup when there is no current browser yet.

Review of attachment 672930 [details] [diff] [review]:
-----------------------------------------------------------------

::: accessible/src/jsat/Utils.jsm
@@ +110,5 @@
>        messageManagers.push(aWindow.messageManager.getChildAt(i));
>  
> +    let document = this.getCurrentContentDoc(aWindow);
> +
> +    if (!document)

Is it worth putting a comment here for the known case?

@@ +111,5 @@
>  
> +    let document = this.getCurrentContentDoc(aWindow);
> +
> +    if (!document)
> +      return [];

If we expected to hit !document often I'd ask if it might be worth putting the get call and the document check at the beginning of this function, but we don't expect this right?

Comment 4

[Eitan Isaacson [:eeejay]]

(In reply to David Bolter [:davidb] from comment #3)
> Comment on attachment 672930 [details] [diff] [review]
> Handle AccessFu startup when there is no current browser yet.
> 
> Review of attachment 672930 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/Utils.jsm
> @@ +110,5 @@
> >        messageManagers.push(aWindow.messageManager.getChildAt(i));
> >  
> > +    let document = this.getCurrentContentDoc(aWindow);
> > +
> > +    if (!document)
> 
> Is it worth putting a comment here for the known case?
> 
> @@ +111,5 @@
> >  
> > +    let document = this.getCurrentContentDoc(aWindow);
> > +
> > +    if (!document)
> > +      return [];
> 
> If we expected to hit !document often I'd ask if it might be worth putting
> the get call and the document check at the beginning of this function, but
> we don't expect this right?

Actually, in Android this is the new normal. So yes, I'll move it to the top of the function.

Comment 5

[Eitan Isaacson [:eeejay]]

Attachment: Bug 803112 - Handle AccessFu startup when there is no current browser yet.

Actually, this seems a bit more correct. We collect message managers for tabs before we check for iframes. So we should not return an empty array.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Comment on attachment 672996 [details] [diff] [review]
Bug 803112 - Handle AccessFu startup when there is no current browser yet.

Review of attachment 672996 [details] [diff] [review]:
-----------------------------------------------------------------

r=me, yeah seems better as long as it tests out ok.

Comment 7

[Marco Zehe (:MarcoZ)]

Comment on attachment 672996 [details] [diff] [review]
Bug 803112 - Handle AccessFu startup when there is no current browser yet.

I can confirm that this fixes the bug for me. I built with this patch locally and tested the build which previously showed the broken behavior, but with this patch, again works fine.

Comment 8

[Eitan Isaacson [:eeejay]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/85425f2440b1

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/85425f2440b1

Comment 10

[Marco Zehe (:MarcoZ)]

Verified fixed in the 2012-10-20 nightly build.

Comment 11

[Marco Zehe (:MarcoZ)]

Comment on attachment 672996 [details] [diff] [review]
Bug 803112 - Handle AccessFu startup when there is no current browser yet.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Lingering, ultimately exposed by service reorg bug, but could hit users anytime.
User impact if declined: An occasionally unusable web content area.
Testing completed (on m-c, etc.): Yes.
Risk to taking this patch (and alternatives if risky): None known.
String or UUID changes made by this patch: None.

Comment 12

[Eitan Isaacson [:eeejay]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/b1a8ae6ff84f