If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Can't dismiss second Twitter photo or video lightbox

VERIFIED FIXED in Firefox 31

Status

()

Core
DOM
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: cpeterson, Assigned: bz)

Tracking

({regression})

unspecified
mozilla33
x86
Mac OS X
regression
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox30 unaffected, firefox31+ verified, firefox32+ verified, firefox33 verified)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
STR:
1. Load https://twitter.com/mozilla
2. Scroll down to the Mozilla account's shared videos.
3. Click on any of the shared videos.
4. After the video has loaded, click on the page's background or the X button to dismiss the video.
5. Click on another video.
6. After the second video has loaded, click on the page's background or the X button to dismiss the video.

RESULT:
Nothing happens. The second video will not close. You must reload the page to interact with Twitter.

This is a regression in Nightly 31 build 2014-04-15. I bisected changesets to this pushlog regression range:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=eacdbceea76d&tochange=17f84a2187f2
(Reporter)

Comment 1

3 years ago
bz: does this Twitter bug look like a regression from your bug 822442 or bug 843840? Regardless, all five bugs in the regression range are yours. :)
Flags: needinfo?(bzbarsky)
(Reporter)

Comment 2

3 years ago
This bug is a significant Twitter regression in Firefox 31.
tracking-firefox31: --- → ?
tracking-firefox32: --- → ?
tracking-firefox31: ? → +
tracking-firefox32: ? → +
Hmm.  I can reproduce the bug, but not in a build from revision 17f84a2187f2...
In fact, I can reproduce in my normal browsing profile with a build from revision 20ca234fd62b (May 12 nightly) but cannot reproduce in that same build with a clean profile.
Flags: needinfo?(bzbarsky)
Aha.  Looks like I have to be logged in to twitter to reproduce the bug.
Not too surprisingly:

The first bad revision is:
changeset:   178744:f0057045ace5
user:        Boris Zbarsky <bzbarsky@mit.edu>
date:        Tue Apr 15 22:58:44 2014 -0400
summary:     Bug 843840 part 2.  Add a way to ask DOM proxies for only their enumerable property names, and use that in the enumerate hook.  r=peterv
Blocks: 843840
The key behavior change here was that before we only enumerated window names for which getOwnPropertyDescriptor returned a descriptor.  But now we're enumerating all window names, including the empty string (which is what breaks twitter) and worse yet the ones we should be hiding because they're changed cross-origin.

Patch coming up to fix this.
Assignee: nobody → bzbarsky
Whiteboard: [need review]
Created attachment 8434261 [details] [diff] [review]
When enumerating the window, make sure to not enumerate frame names that we wouldn't actually expose
Attachment #8434261 - Flags: review?(bobbyholley)
Comment on attachment 8434261 [details] [diff] [review]
When enumerating the window, make sure to not enumerate frame names that we wouldn't actually expose

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

::: dom/base/WindowNamedPropertiesHandler.cpp
@@ +178,5 @@
>    win->GetSupportedNames(names);
> +  // Filter out the ones we wouldn't expose from getOwnPropertyDescriptor.
> +  // We iterate backwards so we can remove things from the list easily.
> +  for (size_t i = names.Length(); i > 0; ) {
> +    --i; // Now we're pointing at the next name we want to look at

Nit - why not hoist this into the loop increment, initialize i to names.Length() - 1, and make the condition |i >= 0|?
Attachment #8434261 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (:bholley) from comment #9)
> ::: dom/base/WindowNamedPropertiesHandler.cpp
> @@ +178,5 @@
> >    win->GetSupportedNames(names);
> > +  // Filter out the ones we wouldn't expose from getOwnPropertyDescriptor.
> > +  // We iterate backwards so we can remove things from the list easily.
> > +  for (size_t i = names.Length(); i > 0; ) {
> > +    --i; // Now we're pointing at the next name we want to look at
> 
> Nit - why not hoist this into the loop increment, initialize i to
> names.Length() - 1, and make the condition |i >= 0|?

size_t is unsigned, so the second wouldn't work.
Yes, exactly, because size_t is unsigned.  Backwards loops with unsigned indices are a pain to write correctly.  :(
https://hg.mozilla.org/integration/mozilla-inbound/rev/8860fd09236a
Flags: in-testsuite+
Whiteboard: [need review]
Comment on attachment 8434261 [details] [diff] [review]
When enumerating the window, make sure to not enumerate frame names that we wouldn't actually expose

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 843840
User impact if declined: Twitter doesn't work right.
Testing completed (on m-c, etc.): Passes tests.
Risk to taking this patch (and alternatives if risky): Risk is pretty low, I
   think.  A maybe-safer alternative is to only filter out empty string names but
   leave cross-origin-inaccessible ones...
String or IDL/UUID changes made by this patch: None
Attachment #8434261 - Flags: approval-mozilla-beta?
Attachment #8434261 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/8860fd09236a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Attachment #8434261 - Flags: approval-mozilla-beta?
Attachment #8434261 - Flags: approval-mozilla-beta+
Attachment #8434261 - Flags: approval-mozilla-aurora?
Attachment #8434261 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/04c3fc11907f
https://hg.mozilla.org/releases/mozilla-beta/rev/2203e70d4b5e
status-firefox31: affected → fixed
status-firefox32: affected → fixed
status-firefox33: --- → fixed
And https://hg.mozilla.org/releases/mozilla-beta/rev/0d1097525746 to fix the build bustage on beta due to the ChildWindow return type change.
(In reply to Josh Matthews from comment #10)
> (In reply to Bobby Holley from comment #9)
> > > +  // We iterate backwards so we can remove things from the list easily.
> > > +  for (size_t i = names.Length(); i > 0; ) {
> > > +    --i; // Now we're pointing at the next name we want to look at
> > 
> > Nit - why not hoist this into the loop increment, initialize i to
> > names.Length() - 1, and make the condition |i >= 0|?
> 
> size_t is unsigned, so the second wouldn't work.

; i-- > 0; ) would work, but is probably even more unreadable.
(There's a joke version ;i --> 0; )
> but is probably even more unreadable.

Yes, indeed.  That's why I didn't do it.
Keywords: verifyme
(Reporter)

Comment 19

3 years ago
Verified fixed on Nightly 33 and Aurora 32.

I can still reproduce the bug on Beta 31, but that's because Release Management has not pushed a new Beta build since this bug was fixed on mozilla-beta. I verified that a Beta 31 build from mozilla-beta's TBPL is fixed.
Status: RESOLVED → VERIFIED
status-firefox31: fixed → verified
status-firefox32: fixed → verified
status-firefox33: fixed → verified
Keywords: verifyme
Duplicate of this bug: 1012109
You need to log in before you can comment on or make changes to this bug.