Last Comment Bug 808243 - sidebar load event could cause sidebar's browser isActive and visibility event to be sent when hidden
: sidebar load event could cause sidebar's browser isActive and visibility even...
Status: RESOLVED FIXED
[qa-]
:
Product: Firefox
Classification: Client Software
Component: SocialAPI (show other bugs)
: 17 Branch
: x86_64 Windows 7
: -- normal (vote)
: Firefox 19
Assigned To: Mark Hammond [:markh]
:
Mentors:
Depends on:
Blocks: 807215 807225
  Show dependency treegraph
 
Reported: 2012-11-02 17:17 PDT by Mark Hammond [:markh]
Modified: 2012-12-04 15:04 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
fixed


Attachments
Check desired state after load fires. (1.30 KB, patch)
2012-11-02 17:17 PDT, Mark Hammond [:markh]
no flags Details | Diff | Splinter Review
Use an alternative strategy - remove the load listener (2.65 KB, patch)
2012-11-04 19:50 PST, Mark Hammond [:markh]
felipc: review+
Details | Diff | Splinter Review
new patch without the setTimeout (2.61 KB, patch)
2012-11-05 15:24 PST, Mark Hammond [:markh]
markh: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Mark Hammond [:markh] 2012-11-02 17:17:12 PDT
Created attachment 677948 [details] [diff] [review]
Check desired state after load fires.

Bug 777177 changed things such that the sidebar isn't actually made visible until the load event for the sidebar content has fired.  If the sidebar is made visible (which adds the load listener), then quickly made invisible before the load event fires, the load event will make the sidebar visible again when it should remain invisible.

A case can probably be made that the "only show after load" behaviour is wrong and a regression, but the fix to this particular case is simple and will fix bug 807225 and bug 807215
Comment 1 Mark Hammond [:markh] 2012-11-02 17:27:11 PDT
oops - it's not the actual visibility that is affected, just the docShell state and the firing of the custom visibility event.
Comment 2 Mark Hammond [:markh] 2012-11-04 19:50:23 PST
Created attachment 678196 [details] [diff] [review]
Use an alternative strategy - remove the load listener

After thinking about this, the old patch almost certainly would work fine - however, it isn't great as in psychopathic cases we could still end up with multiple load listeners firing and in theory could send multiple socialFrameShow() message to the content without socialFrameHide in between.

A more robust approach is simply to remove the load listener as soon as we make it hidden.

Try run at https://tbpl.mozilla.org/?tree=Try&rev=a56c83de3a4d, and I'll try and remember to retrigger the job a number of times to increase confidence it does actually solve the oranges.
Comment 3 Mark Hammond [:markh] 2012-11-04 22:09:17 PST
Comment on attachment 678196 [details] [diff] [review]
Use an alternative strategy - remove the load listener

hrmph - still got orange with this patch - https://tbpl.mozilla.org/php/getParsedLog.php?id=16747875&tree=Try

new try with more diagnotics: https://tbpl.mozilla.org/?tree=Try&rev=f3992017649a
Comment 4 Mark Hammond [:markh] 2012-11-05 04:25:17 PST
Comment on attachment 678196 [details] [diff] [review]
Use an alternative strategy - remove the load listener

doh! Problem is the setTimeout in the load listener, so the same "race" happens there.  The setTimeout isn't necessary, so I still think this is the right approach.

try without the setTimeout: https://tbpl.mozilla.org/?tree=Try&rev=b0b56006cbcc

So back for review assuming no setTimeout...
Comment 5 :Felipe Gomes (needinfo me!) 2012-11-05 15:22:07 PST
Comment on attachment 678196 [details] [diff] [review]
Use an alternative strategy - remove the load listener

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

r+ without the setTimeout.. thanks for investigating this!

after you land can you post an updated patch so there's no risk someone grabs this one to land on a branch in case we uplift it?
Comment 6 Mark Hammond [:markh] 2012-11-05 15:24:23 PST
Created attachment 678502 [details] [diff] [review]
new patch without the setTimeout

the one to land.
Comment 8 Ed Morley [:emorley] 2012-11-06 06:11:51 PST
https://hg.mozilla.org/mozilla-central/rev/c697c428739d
Comment 9 :Felipe Gomes (needinfo me!) 2012-11-06 15:26:46 PST
Comment on attachment 678502 [details] [diff] [review]
new patch without the setTimeout

[Approval Request Comment]
Bug caused by (feature/regressing bug #): social api
User impact if declined: should help clear an intermittent test on aurora/beta that is more easily triggered by the private-browsing test (bug 807217)
Testing completed (on m-c, etc.): landed on m-c
Risk to taking this patch (and alternatives if risky): simple change
String or UUID changes made by this patch: none
Comment 10 Lukas Blakk [:lsblakk] use ?needinfo 2012-11-06 16:40:13 PST
Comment on attachment 678502 [details] [diff] [review]
new patch without the setTimeout

low risk, needed for better test results on what we're trying to ship, so approving for uplift.
Comment 11 Ryan VanderMeulen [:RyanVM] 2012-11-06 18:36:29 PST
https://bugzilla.mozilla.org/show_bug.cgi?id=808243
Comment 12 :Felipe Gomes (needinfo me!) 2012-11-06 18:40:16 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/4547ff67ebb0
Comment 13 :Felipe Gomes (needinfo me!) 2012-11-06 19:33:27 PST
https://hg.mozilla.org/releases/mozilla-aurora/rev/04ecbd5b2b27
Comment 14 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-12-04 15:04:03 PST
Please remove [qa-] whiteboard tag and add verifyme keyword if there's some QA testing needed here. Otherwise we will skip verification.

Note You need to log in before you can comment on or make changes to this bug.