1019417 - Can't dismiss second Twitter photo or video lightbox

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Chris Peterson [:cpeterson]]

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

Comment 1

[Chris Peterson [:cpeterson]]

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. :)

Comment 2

[Chris Peterson [:cpeterson]]

This bug is a significant Twitter regression in Firefox 31.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Hmm.  I can reproduce the bug, but not in a build from revision 17f84a2187f2...

Comment 4

[Boris Zbarsky [:bzbarsky]]

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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Aha.  Looks like I have to be logged in to twitter to reproduce the bug.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Not too surprisingly:

The first bad revision is:
changeset:   178744:f0057045ace5
user:        Boris Zbarsky <[e-mail]>
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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: When enumerating the window, make sure to not enumerate frame names that we wouldn't actually expose

Comment 9

[Bobby Holley (:bholley)]

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|?

Comment 10

[Josh Matthews [:jdm]]

(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.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Yes, exactly, because size_t is unsigned.  Backwards loops with unsigned indices are a pain to write correctly.  :(

Comment 12

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8860fd09236a

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8860fd09236a

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/04c3fc11907f
https://hg.mozilla.org/releases/mozilla-beta/rev/2203e70d4b5e

Comment 16

[Boris Zbarsky [:bzbarsky]]

And https://hg.mozilla.org/releases/mozilla-beta/rev/0d1097525746 to fix the build bustage on beta due to the ChildWindow return type change.

Comment 17

[[e-mail]]

(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; )

Comment 18

[Boris Zbarsky [:bzbarsky]]

> but is probably even more unreadable.

Yes, indeed.  That's why I didn't do it.

Comment 19

[Chris Peterson [:cpeterson]]

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.