Closed Bug 808243 Opened 7 years ago Closed 7 years ago

sidebar load event could cause sidebar's browser isActive and visibility event to be sent when hidden

Categories

(Firefox Graveyard :: SocialAPI, defect)

17 Branch
x86_64
Windows 7
defect
Not set

Tracking

(firefox17 fixed, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
Firefox 19
Tracking Status
firefox17 --- fixed
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 2 obsolete files)

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
Attachment #677948 - Flags: review?(felipc)
oops - it's not the actual visibility that is affected, just the docShell state and the firing of the custom visibility event.
Summary: sidebar load event could cause sidebar visibility to be set incorrectly. → sidebar load event could cause sidebar's browser isActive and visiblity event to be sent when hidden
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.
Attachment #677948 - Attachment is obsolete: true
Attachment #677948 - Flags: review?(felipc)
Attachment #678196 - Flags: review?(felipc)
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
Attachment #678196 - Flags: review?(felipc)
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...
Attachment #678196 - Flags: review?(felipc)
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?
Attachment #678196 - Flags: review?(felipc) → review+
the one to land.
Assignee: nobody → mhammond
Attachment #678196 - Attachment is obsolete: true
Attachment #678502 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/c697c428739d
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
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
Attachment #678502 - Flags: approval-mozilla-beta?
Attachment #678502 - Flags: approval-mozilla-aurora?
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.
Attachment #678502 - Flags: approval-mozilla-beta?
Attachment #678502 - Flags: approval-mozilla-beta+
Attachment #678502 - Flags: approval-mozilla-aurora?
Attachment #678502 - Flags: approval-mozilla-aurora+
Summary: sidebar load event could cause sidebar's browser isActive and visiblity event to be sent when hidden → sidebar load event could cause sidebar's browser isActive and visibility event to be sent when hidden
Please remove [qa-] whiteboard tag and add verifyme keyword if there's some QA testing needed here. Otherwise we will skip verification.
Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.