Account Manager threading badness

[donaddon]

Hello,

I've been running into a variety of problems caused by the Account
Manager extension. I traced them back to the synchronous emulation
routine that uses Thread.processNextEvent.

This is causing re-entrancy in all kinds of code that might not expect
it. Badness ensues. Here's an example:

1. I have an extension installed that calls removeChild on a <browser>
element. This may happen before the browser element has finished
loading (in this case it's an invisible browser element for background
loading).
2. This results in an nsIWebProgressListener onChangeState(STATE_STOP)
event being generated.
3. The Account Manager handles this event and goes into one of these
Sync wait states while trying to update realm information.
4. The hidden browser's nsDocShell is only partially torn down and the
request has not yet been cancelled. And since we are still processing
code, the hidden browser element's load operation may complete.
5. The URI Loader asks the hidden nsDocShell to handle the content,
but because of its partially torn-down state it decides not to, and
instead a higher-level content listener handles the event.
6. The higher level content listener decides to create a new tab to
display the content.

End Result: Suprious tabs start showing up while you are using the
Account Manager.

It strikes me as a fundamentally bad idea to make javascript code re-
entrant in this way, especially during processing of
nsIWebProgressListenerevents.

Thanks!

[donaddon]

I've got a repro. Is there a public bug database? Here are the
steps:

1. Install the account manager.
2. Run the script below in a protected chrome context. You can run it
directly in the browser.xul context using the extension developer
extension / javascript shell / enumerateWindows / browser.xul context.
3. With the account manager disabled, the script is harmless, with it
enabled, you will get a whole bunch of tabs opened.
4. You have to restart the browser every time to reproduce:

// The otherwise harmless script
var testUrls = [
"http://www.urlpass.com/",
"http://www.polldaddy.com/",
"http://www.communityspark.com/dont-lose-existing-members-of-your-
online-community/",
"http://www.maps.google.com/",
"http://www.yahoo.com/",
"http://us.mcafee.com/root/speedometer/default.asp",
"http://www22.verizon.com/content/verizonglobalhome/
ghp_landing.aspx",
"http://www.bandwidthplace.com/",
"http://fisherbikes.com/",
"http://www.trekbikes.com/us/en/",
"http://www.konaworld.com/"
]

function troubleLoop() {
var nextUrl = function(iUrl) {
if(iUrl == testUrls.length)
{
alert("done!");
return;
}

// Remove the current browser if it exists
var browser = document.getElementById("theBrowser22");
if(browser)
{
document.getElementById("main-window").removeChild(browser);
}
browser = document.createElement("browser");
browser.setAttribute("id", "theBrowser22");
browser.setAttribute("type", "content");
browser.setAttribute("src", testUrls[iUrl]);
browser.setAttribute("style",
"visibility:hidden;overflow:auto;border:0px solid black;background-
color:white;width:0px;height:0px;");
document.getElementById("main-window").appendChild(browser);

window.setTimeout(function() { nextUrl(iUrl+1); }, 200);
}
nextUrl(0);
}

troubleLoop();

[Mike Hanson]

Dan: Are we still using the Weave product/Identity component to track bugs?

(That would be https://bugzilla.mozilla.org/enter_bug.cgi?full=1 then click "Weave" and choose the "Identity" component)

-m

[Ozten]

On May 6, 2:25 pm, donaddon <dons...@hotmail.com> wrote:
> I've got a repro.  Is there a public bug database?  Here are the

https://bugzilla.mozilla.org/enter_bug.cgi?product=Weave&component=Identity

[Dan Mills]

I filed a bug:

https://bugzilla.mozilla.org/show_bug.cgi?id=564295

Feel free to add more info there; I also linked to this thread.

Dan

[donaddon]

I know a bug has been entered, but I don't know who prioritizes these,
and that implementation is trouble. It's already causing mysterious
problems and conflicts and I think it's going to cause many more if
the add-on gains popularity. Can we get that bug escalated?

Thanks!
- Don

On May 6, 2:36 pm, Dan Mills <thun...@mozilla.com> wrote:
> I filed a bug:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=564295
>
> Feel free to add more info there; I also linked to this thread.
>
> Dan
>

[Dan Mills]

I suspect changes will be nontrivial, and we are going to be rewriting Account Manager soon anyway, so spending a ton of time on the add-on is wasted time we could be spending on the rewrite.

However, patches are welcome.  I volunteer to review any that get attached to that bug :)  Ideally the fix goes into resource.js and makes available an async interface, or otherwise minimally changes the resource.js api.

Dan

[Don Schmitt]

Thanks for the heads up that a rewrite is pending.  I look forward to seeing the first iteration, but will consider submitting a patch if the current Account Manager gets much more popular.

 

Thanks!

-          Don

 

 

From: mozilla-labs-o...@googlegroups.com [mailto:mozilla-labs-o...@googlegroups.com] On Behalf Of Dan Mills
Sent: Tuesday, May 18, 2010 9:30 PM
To: mozilla-labs-o...@googlegroups.com
Subject: Re: Account Manager threading badness

 

I suspect changes will be nontrivial, and we are going to be rewriting Account Manager soon anyway, so spending a ton of time on the add-on is wasted time we could be spending on the rewrite.

However, patches are welcome.  I volunteer to review any that get attached to that bug :)  Ideally the fix goes into resource.js and makes available an async interface, or otherwise minimally changes the resource.js api.

Dan

On Tue, May 18, 2010 at 2:29 PM, donaddon <don.sc@verizon.net> wrote:

I know a bug has been entered, but I don't know who prioritizes these,

and that implementation is trouble.    It's already causing mysterious

problems and conflicts and I think it's going to cause many more if

the add-on gains popularity.  Can we get that bug escalated?

Thanks!
 - Don

On May 6, 2:36 pm, Dan Mills <thun...@mozilla.com> wrote:
> I filed a bug:
>
> https://bugzilla.mozilla.org/show_bug.cgi?id=564295
>
> Feel free to add more info there; I also linked to this thread.
>
> Dan
>

> On Thu, May 6, 2010 at 2:35 PM, Mike Hanson <mhan...@mozilla.com> wrote:
> > Dan: Are we still using the Weave product/Identity component to track bugs?
>

> > (That would behttps://bugzilla.mozilla.org/enter_bug.cgi?full=1then

> > click "Weave" and choose the "Identity" component)
>
> > -m
>
> > On May 6, 2010, at 2:25 PM, donaddon wrote:
>

> > > I've got a repro.  Is there a public bug database?  Here are the

> > > steps:
>
> > > 1. Install the account manager.

> > > 2. Run the script below in a protected chrome context.  You can run it

> > > directly in the browser.xul context using the extension developer
> > > extension / javascript shell / enumerateWindows / browser.xul context.
> > > 3. With the account manager disabled, the script is harmless, with it
> > > enabled, you will get a whole bunch of tabs opened.
> > > 4. You have to restart the browser every time to reproduce:
>
> > > // The otherwise harmless script
> > > var testUrls = [

> > >       "http://www.urlpass.com/",
> > >       "http://www.polldaddy.com/",
> > >       "http://www.communityspark.com/dont-lose-existing-members-of-your-
> > > online-community/",
> > >       "http://www.maps.google.com/",
> > >       "http://www.yahoo.com/",
> > >       "http://us.mcafee.com/root/speedometer/default.asp",
> > >       "http://www22.verizon.com/content/verizonglobalhome/
> > > ghp_landing.aspx",
> > >       "http://www.bandwidthplace.com/",
> > >       "http://fisherbikes.com/",
> > >       "http://www.trekbikes.com/us/en/",
> > >       "http://www.konaworld.com/"
> > > ]
>
> > > function troubleLoop() {
> > >       var nextUrl = function(iUrl) {
> > >               if(iUrl == testUrls.length)
> > >               {
> > >                       alert("done!");
> > >                       return;
> > >               }
>
> > >               // Remove the current browser if it exists
> > >               var browser = document.getElementById("theBrowser22");
> > >               if(browser)
> > >               {
>
> > document.getElementById("main-window").removeChild(browser);
> > >               }
> > >               browser = document.createElement("browser");
> > >               browser.setAttribute("id", "theBrowser22");
> > >               browser.setAttribute("type", "content");
> > >               browser.setAttribute("src", testUrls[iUrl]);
> > >               browser.setAttribute("style",

> > > "visibility:hidden;overflow:auto;border:0px solid black;background-
> > > color:white;width:0px;height:0px;");
>
> > document.getElementById("main-window").appendChild(browser);
>

> > >               window.setTimeout(function() { nextUrl(iUrl+1); }, 200);
> > >       }
> > >       nextUrl(0);

> > > }
>
> > > troubleLoop();
>
> > > On May 6, 1:14 pm, donaddon <dons...@hotmail.com> wrote:
> > >> Hello,
>
> > >> I've been running into a variety of problems caused by the Account

> > >> Manager extension.  I traced them back to the synchronous emulation

> > >> routine that uses Thread.processNextEvent.
>
> > >> This is causing re-entrancy in all kinds of code that might not expect

> > >> it.  Badness ensues.  Here's an example:

>
> > >> 1. I have an extension installed that calls removeChild on a <browser>

> > >> element.  This may happen before the browser element has finished

> > >> loading (in this case it's an invisible browser element for background
> > >> loading).
> > >> 2. This results in an nsIWebProgressListener onChangeState(STATE_STOP)
> > >> event being generated.
> > >> 3. The Account Manager handles this event and goes into one of these
> > >> Sync wait states while trying to update realm information.
> > >> 4. The hidden browser's nsDocShell is only partially torn down and the

> > >> request has not yet been cancelled.  And since we are still processing

> > >> code, the hidden browser element's load operation may complete.
> > >> 5. The URI Loader asks the hidden nsDocShell to handle the content,
> > >> but because of its partially torn-down state it decides not to, and
> > >> instead a higher-level content listener handles the event.
> > >> 6. The higher level content listener decides to create a new tab to
> > >> display the content.
>

> > >> End Result:  Suprious tabs start showing up while you are using the