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

VERIFIED FIXED in Firefox 18

Status

()

--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: MarcoZ, Assigned: eeejay)

Tracking

({regression})

Trunk
mozilla19
ARM
Android
regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
Seems to still work on GB, but not on JB.
(Assignee)

Comment 2

6 years ago
Created attachment 672930 [details] [diff] [review]
Handle AccessFu startup when there is no current browser yet.
Attachment #672930 - Flags: review?(dbolter)
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?
Attachment #672930 - Flags: review?(dbolter) → review+
(Assignee)

Comment 4

6 years ago
(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.
(Assignee)

Comment 5

6 years ago
Created attachment 672996 [details] [diff] [review]
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.
Attachment #672930 - Attachment is obsolete: true
Attachment #672996 - Flags: review?(dbolter)
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.
Attachment #672996 - Flags: review?(dbolter) → review+
(Reporter)

Comment 7

6 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/85425f2440b1
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(Reporter)

Comment 10

6 years ago
Verified fixed in the 2012-10-20 nightly build.
Status: RESOLVED → VERIFIED
(Reporter)

Comment 11

6 years ago
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.
Attachment #672996 - Flags: approval-mozilla-aurora?

Updated

6 years ago
Attachment #672996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox18: --- → fixed
status-firefox19: --- → fixed
You need to log in before you can comment on or make changes to this bug.