Closed Bug 914435 Opened 9 years ago Closed 9 years ago

some updates from one provider will affect all providers


(Firefox Graveyard :: SocialAPI, defect)

26 Branch
Not set


(Not tracked)

Firefox 27


(Reporter: mixedpuppy, Assigned: mixedpuppy)




(1 file, 2 obsolete files)

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.
Assignee: nobody → mixedpuppy
Attachment #804057 - Flags: review?(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
Attachment #804057 - Flags: review?(mhammond) → review+
feeback and new try
Attachment #804057 - Attachment is obsolete: true
Attachment #804685 - Flags: review+
see if I get a clean try after some leaky pushes were backed out
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 26
Backed out in 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.
Resolution: FIXED → ---
Target Milestone: Firefox 26 → ---
Depends on: 916757
this looks like the crash culprit.
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.

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
Attachment #804685 - Attachment is obsolete: true
Attachment #811342 - Flags: review?(mhammond)
Attachment #811342 - Flags: review?(mhammond) → review+
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.