914435 - some updates from one provider will affect all providers

Status: RESOLVED FIXED

(Firefox Graveyard :: SocialAPI, defect)

Description

[Shane Caraveo (:mixedpuppy)]

A bug in one providers worker is setting the user profile every second (bug reported to them).  This results in the ui being updated and removing all iframes used in the notification panel, which now may contain iframes from more than one provider.  In worse case scenario, you have a panel open at the time this happens.  The iframe is removed and the panel becomes a little stub (on osx, probably gone on windows).  This happens in SocialToolbar._updateButtonHiddenState where it removes the SharedFrame groups for all iframes.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: fix handling of frames for ambient icons

Comment 2

[Shane Caraveo (:mixedpuppy)]

https://tbpl.mozilla.org/?tree=Try&rev=cec3bd224ed9

Comment 3

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 804057 [details] [diff] [review]
fix handling of frames for ambient icons

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

::: browser/base/content/test/social/browser_social_toolbar.js
@@ +198,5 @@
> +    let numIcons = Object.keys(Social.provider.ambientNotificationIcons).length;
> +    let ambientIcons = document.querySelectorAll("#social-toolbar-item > toolbarbutton[type='badged']");
> +    is(numIcons, ambientIcons.length, "all ambient icons exist");
> +    is(panel.childNodes.length, ambientIcons.length + 1, "frames all exist");
> +    

whitespace here and at line 209

@@ +207,5 @@
> +        let icons = document.querySelectorAll("#social-toolbar-item > toolbarbutton[type='badged']");
> +        is(icons.length, 0, "ambient icons have been removed");
> +    
> +        // is the status button frame for provider 2 still there?
> +        for (let f of panel.childNodes) {

this block looks like debugging and should either be removed, or the info changed to an ok/is check

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: fix handling of frames for ambient icons

feeback and new try

https://tbpl.mozilla.org/?tree=Try&rev=3b9340db5ad5

Comment 5

[Shane Caraveo (:mixedpuppy)]

see if I get a clean try after some leaky pushes were backed out

https://tbpl.mozilla.org/?tree=Try&rev=2fbfa4d9c2e6

Comment 6

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/a04d4a573863

Comment 7

[Ryan VanderMeulen [:RyanVM]]

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

Comment 8

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/ee7366ad4698 while chasing bug 916757.

Feel free to reland after a debug Windows try push with like 30 browser-chrome runs on WinXP and Win8, since we're still not especially sure what got rid of it.

Comment 9

[Shane Caraveo (:mixedpuppy)]

new winxp/8 try

https://tbpl.mozilla.org/?tree=Try&rev=4ad214d5ee9f

Comment 10

[Shane Caraveo (:mixedpuppy)]

winxp/8 try with bug 904104, bug  914926, bug 914927 and bug 914435

https://tbpl.mozilla.org/?tree=Try&rev=6d7e6966f505

Comment 11

[Shane Caraveo (:mixedpuppy)]

this looks like the crash culprit.

https://tbpl.mozilla.org/?tree=Try&rev=a1716c65ed3c

Comment 12

[Mark Hammond [:markh] [:mhammond]]

I can't see what could cause this, although it looks like I can repro it locally on about 1 in 10 runs of just the browser/base/content directory.

It's probably worth experimenting with more try pushed - eg, avoid creating a worker when remote frameworkers are enabled (which you already suggested in IRC), and I see lots of assertions re https:// in the content process, so maybe even dropping https:// test providers from the patch.

I suspect we are going to need help here, so maybe we should continue the conversation in bug  	916757 once we have narrowed it down as far as we can.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Attachment: fix handling of frames for multiple workers

https://tbpl.mozilla.org/?tree=Try&rev=49b4c32280ba

This avoids the crash in bug 916757 by not using the remote frameworker in the test.

Part of the patch (removed) was obsoleted by bug 919803

Comment 14

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/fx-team/rev/3fb7c71990d0

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3fb7c71990d0