Closed Bug 1008615 Opened 5 years ago Closed 5 years ago

Unable to select RSS handler other than Live Bookmarks (nothing changes the selection, only "choose application" works)

Categories

(Firefox Graveyard :: RSS Discovery and Preview, defect)

31 Branch
x86_64
All
defect
Not set

Tracking

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

VERIFIED FIXED
Firefox 32
Tracking Status
firefox30 --- unaffected
firefox31 + verified
firefox32 + verified

People

(Reporter: brad, Assigned: bholley)

References

Details

(Keywords: regression)

Attachments

(2 files)

I have three RSS handlers registered: My Yahoo!, feedly, and Tiny Tiny RSS. But no matter which I select, Firefox always displays the Live Bookmarks dialog box.

Steps to reproduce:
1. In options under Applications, make sure "Web feed" is set to "Preview with Firefox"
1. Navigate to https://blog.mozilla.org/feed/
2. Select any handler besides Live Bookmarks, such a My Yahoo!
3. Subscribe

Actual result:
The "Subscribe with Live Bookmark" dialog box appears.

Expected result:
Feed should be passed to the selected handler.


Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
I was able to reproduce this in Aurora 31.0a2 (2014-05-15).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: firefox-backlog+
-g 2014-04-07-03-02-03-mozilla-central-firefox-31.0a1.en-US.linux-x86_64 5405d6f4e3c6
-b 2014-04-08-03-02-05-mozilla-central-firefox-31.0a1.ru.linux-x86_64 8883360b1edb

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5405d6f4e3c6&tochange=8883360b1edb
OS: Windows 7 → All
Summary: Unable to select RSS handler other than Live Bookmarks → Unable to select RSS handler other than Live Bookmarks (nothing changes the selection, only "choose application" works)
unfortunately the range is still too large... the only suspect related to feeds is

a0a5af9959de	Bobby Holley — Bug 986730 - Run the FeedWriter sandbox with an expanded principal. r=mrbkap

but hard to tell without trying a local backout
Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8494153485ee
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140406113125
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c9585852983b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0 ID:20140406123325
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=8494153485ee&tochange=c9585852983b
Last Good: c4773f208050
First Bad: ec8a5ce1d620

Triggered by:
ec8a5ce1d620	Bobby Holley — Bug 986730 - Put all anonymous content into the XBL scope. r=smaug
It's almost certainly the FeedWriter patch from bug 986730. Mano, are you able to look at this? If not, can you suggest someone?
Flags: needinfo?(mano)
I wouldn't do that to anyone else. Not that cruel.
Assignee: nobody → mano
Flags: needinfo?(mano)
OK, so, what seems to happen is that either the <menulist> binding isn't attached at all, or that its event handles (<handler>s and onfoo attributes in menulist.xml) aren't attached. Thus, the oncommand handler in menulist.xml, that sets the selectedItem internally is never triggered, and the "selected" attribute is not removed from the "live bookmarks" item.

Event handles set by FeedWriter.js are intact, by the way.

So, all in all, it seems this shouldn't have much to do with the FeedWriter changes, but rather with the xbl changes implemented in bug 986730.
I've also tested the following:
1) removed any manipulation to the menulist from FeedWrtier
2) added another "static" item to the menulist subscribe.xml
3) verified that the menulist is broken the same way.

Notice that when you select "Yahoo" or any other item, it becomes selected (that happens in the menuitem binding, IIRC), but the live bookmark item remains selected as well, meaning that menulist's oncommand is not called. My guess is the event-bubbling is broken. I'd consider that a probable regression of making the entire tree anonymous.
Flags: needinfo?(bobbyholley)
Not a bug in FeedWriter after all (I'd provide a test case, but without remote xul that's quite a challenge).
Assignee: mano → nobody
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #10)
> Not a bug in FeedWriter after all (I'd provide a test case, but without
> remote xul that's quite a challenge).

I'm still only vaguely following what the issue is. Can you provide a testcase in the form of a mochitest in dom/xbl/test (which are hooked up to allow remote XUL)? Once we have that, I can make sure this bug gets fixed (possibly with smaug's help).
Flags: needinfo?(bobbyholley) → needinfo?(mano)
There are various mochitest issues that make writing a true test painful, e.g. document.getAnonymousElementByAttribute is not accessible in the test; binding methods are inaccessible either; <field>s seem to work, but the returned anonymous elements are blocked somehow (getAttribute requires privileges). I could probably switch to mochitest-chrome, but that differs from the subscribe.xhtml case (which runs with content privileges). 

So consider this "test" a way to open a remote-xul document. It just keeps the window open (SimpleTest.finish isn't called) so you can see what happens.

I figured out that having the menulist inside a binding (as we do in subscribe.xhtml via subscribe.xml) makes a key difference here. In the "test" you have two menulists, one within a binding, and one in the xhtml document itself. The xhtml one is broken for other reasons, and probably should not be investigated here.

The binding menulist reproduces the exact problem I see in subscribe.xhtml:

./mach mochitest-plain content/xul/content/test/test_remote_xul_menulist.xhtml 

1) Open the menulist
2) Notice that "First In Binding" is already selected.
3) Select "Second In Binding"
4) Reopen the menu.
5) Notice that both items are selected.

Either some event handler isn't called, or something else in the binding is broken. For what it's worth, when I open the menulist I see an error in the popupshowing handler "this.menuBoxObject is undefined". I don't see a similar error when I select an item, but I would expect a missing menuBoxObject to break stuff.
Flags: needinfo?(mano)
(sorry spam, about this.menuBoxObject is undefined , I had already filed Bug 1016127)
So, the issue here is that the semantics of <field>s make them pretty much impossible to implement securely for in-content XBL. We now run in-content XBL in a separate scope that gets XrayWrappers to the content itself. And since <field>s must be a data property on the target node (visible to the content), there's no way to access that property both content and XBL without having XBL waive Xrays.


Bug 843829 demonstrated that waiving was a bad idea. So I removed <field>s from all of the in-content XBL bindings I was aware of in bug 848939. But I didn't know about FeedWriter (I guess it doesn't really have test coverage?), so I missed menulist.xml.

I don't know why this didn't break with bug 843829, but the fix should be the same - just emulate the behavior of <field> in the <constructor>. I'll attach a patch.
Mano, this patch fixes your testcase. Can you check whether it fixes the
original bug here?
Attachment #8431235 - Flags: feedback?(mano)
Comment on attachment 8431235 [details] [diff] [review]
De-field menulist.xml. v1

It does.
Attachment #8431235 - Flags: feedback?(mano) → feedback+
Comment on attachment 8431235 [details] [diff] [review]
De-field menulist.xml. v1


># HG changeset patch
># User Bobby Holley <bobbyholley@gmail.com>
>
>Bug 1008615 - De-field menulist.xml. v1
>
>diff --git a/toolkit/content/widgets/menulist.xml b/toolkit/content/widgets/menulist.xml
>index b956cda..68e58d6 100644
>--- a/toolkit/content/widgets/menulist.xml
>+++ b/toolkit/content/widgets/menulist.xml
>@@ -63,17 +63,20 @@
>             }
>           }
>         ]]>
>       </handler>
>     </handlers>
> 
>     <implementation implements="nsIDOMXULMenuListElement, nsIDOMEventListener">
>       <constructor>
>-        this.setInitialSelection()
>+        this.mInputField = null;
>+        this.mSelectedInternal = null;

>+        this.menuBoxObject = this.boxObject.QueryInterface(Components.interfaces.nsIMenuBoxObject);

The field was |readonly|. It should thus be not-writable, not configurable.

In fact, is it possible to |delete| fields? If not, at least technically they should all be not-configurable.
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #18)
> The field was |readonly|. It should thus be not-writable, not configurable.

I considered using Object.defineProperty to make it read-only, but that seemed like overkill. There doesn't seem to be an immediate threat of anybody overwriting this (especially since the values are Xray expandos and not visible to content). My interpretation of the readonly attribute was more of a "eh, why not?" sort of thing. I can do the Object.defineProperty gymnastics if you like though.

> In fact, is it possible to |delete| fields? If not, at least technically
> they should all be not-configurable.

They are configurable per the implementation (see the lack of JSPROP_PERMANENT on mJSAttributes in nsXBLProtoImplField.cpp), and the implementation in dom/xbl is the de facto spec at this point. 

I'm going to flag you for review. Let me know if you're not the appropriate reviewer.
Attachment #8431235 - Flags: review?(mano)
(In reply to Bobby Holley (:bholley) from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=e0fec9681fc2

For posterity - this was green.
Comment on attachment 8431235 [details] [diff] [review]
De-field menulist.xml. v1

Appropriate reviewer.

I would mark it readonly (and if you so do so after all, set enumerable: true), but r=me regardless. I agree that it's minor.
Attachment #8431235 - Flags: review?(mano) → review+
I'm just going to push the patch that had a green try push:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d271a898515b
Comment on attachment 8431235 [details] [diff] [review]
De-field menulist.xml. v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 986730 
User impact if declined: live feed reading is broken
Testing completed (on m-c, etc.): just pushed to m-i
Risk to taking this patch (and alternatives if risky): medium-low risk. No lower-risk alternatives.
String or IDL/UUID changes made by this patch: none
Attachment #8431235 - Flags: approval-mozilla-aurora?
Assignee: nobody → bobbyholley
Bobby, I suppose a mochitext here is too painful for the same issues I run into?
(In reply to Mano (needinfo? for any questions; not reading general bugmail) from comment #24)
> Bobby, I suppose a mochitext here is too painful for the same issues I run
> into?

Not necessarily, no. But what we actually need here is test coverage for the Feed functionality that broke (possibly as a browser-chrome test).
Yeah, I filed bug 1018055.

It would be nice to just test that xul:menulist is functional in xhtml, or even better: all the de-field-ed bindings. Not a blocker though.
https://hg.mozilla.org/mozilla-central/rev/d271a898515b
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 32
Comment on attachment 8431235 [details] [diff] [review]
De-field menulist.xml. v1

In case of issue, we would have time to revert it.
Attachment #8431235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
QA Whiteboard: [good first verify]
Verified on Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:31.0) Gecko/20100101 Firefox/31.0,  20140619131915 build.
QA Whiteboard: [good first verify] → [good first verify] [testday-20140620]
Whiteboard: p=0 s=33.1 [qa?]
Iteration: --- → 33.2
QA Whiteboard: [good first verify] [testday-20140620] → [good first verify] [testday-20140620] [qa?]
Whiteboard: p=0 s=33.1 [qa?]
QA Whiteboard: [good first verify] [testday-20140620] [qa?] → [good first verify] [testday-20140620] [qa+]
QA Contact: catalin.varga
Verified as fixed using the following environment:

FF Aurora 32
Build Id:20140624004001
OS: Win 7 x64, Mac Os 10.7.5, Ubuntu 13.04 x64
Status: RESOLVED → VERIFIED
QA Whiteboard: [good first verify] [testday-20140620] [qa+] → [good first verify] [testday-20140620] [qa!]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.