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

RESOLVED FIXED in Firefox 17

Status

()

Firefox
SocialAPI
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: markh, Assigned: markh)

Tracking

17 Branch
Firefox 19
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox17 fixed, firefox18 fixed, firefox19 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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
Attachment #677948 - Flags: review?(felipc)
(Assignee)

Comment 1

5 years ago
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
(Assignee)

Comment 2

5 years ago
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.
Attachment #677948 - Attachment is obsolete: true
Attachment #677948 - Flags: review?(felipc)
Attachment #678196 - Flags: review?(felipc)
(Assignee)

Comment 3

5 years ago
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)
(Assignee)

Comment 4

5 years ago
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+
(Assignee)

Comment 6

5 years ago
Created attachment 678502 [details] [diff] [review]
new patch without the setTimeout

the one to land.
Assignee: nobody → mhammond
Attachment #678196 - Attachment is obsolete: true
Attachment #678502 - Flags: review+
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c697c428739d

Comment 8

5 years ago
https://hg.mozilla.org/mozilla-central/rev/c697c428739d
Status: NEW → RESOLVED
Last Resolved: 5 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+
status-firefox17: --- → affected
status-firefox18: --- → affected
https://bugzilla.mozilla.org/show_bug.cgi?id=808243
status-firefox17: affected → fixed
status-firefox19: --- → fixed
https://hg.mozilla.org/releases/mozilla-beta/rev/4547ff67ebb0
https://hg.mozilla.org/releases/mozilla-aurora/rev/04ecbd5b2b27
status-firefox18: affected → fixed

Updated

5 years ago
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-]
You need to log in before you can comment on or make changes to this bug.