[FFE] Update WebApps to support Firefox

[Alex Launi]

This FFE corresponds to completing the support for WebApps in Firefox. A Firefox v15 had to be patched in order to support accessing the XID of a given tab window (functionality needed by WebApps). The functionality was added by upstream in v17.

This patch also has impacts in the Firefox extension packages associated with WebApps that have to be updated to support both FF 15 and 17 XID retrieval mechanisms.

Informations about the Firefox patching efforts:
------------------------------------------------

For the unity webapps firefox extension we need access to the XID of the tab window. A patch has been accepted into Firefox 17 by Mozilla exposing this functionality. We are begging for it to be distro patched into Firefox 15 for quantal. The patch and discussion around it are available at https://bugzilla.mozilla.org/show_bug.cgi?id=760802

ProblemType: Bug
DistroRelease: Ubuntu 12.10
Package: firefox 15.0~b5+build1-0ubuntu1
ProcVersionSignature: Ubuntu 3.5.0-11.11-generic 3.5.2
Uname: Linux 3.5.0-11-generic x86_64
NonfreeKernelModules: wl
AddonCompatCheckDisabled: False
ApportVersion: 2.5.1-0ubuntu1
Architecture: amd64
AudioDevicesInUse:
 USER PID ACCESS COMMAND
 /dev/snd/controlC0: alex 1677 F.... pulseaudio
 /dev/snd/pcmC0D0p: alex 1677 F...m pulseaudio
BuildID: 20120820141800
CRDA: Error: command ['iw', 'reg', 'get'] failed with exit code 1: nl80211 not found.
Channel: Unavailable
Date: Wed Aug 22 18:05:36 2012
ForcedLayersAccel: False
IfupdownConfig:
 auto lo
 iface lo inet loopback
InstallationMedia: Ubuntu 12.04 LTS "Precise Pangolin" - Release amd64 (20120425)
IpRoute:
 default via 192.168.2.1 dev eth2 proto static
 169.254.0.0/16 dev eth2 scope link metric 1000
 192.168.2.0/24 dev eth2 proto kernel scope link src 192.168.2.2 metric 9
PrefSources: prefs.js
ProcEnviron:
 TERM=xterm
 PATH=(custom, user)
 LANG=en_US.UTF-8
 SHELL=/bin/bash
Profiles: Profile0 (Default) - LastVersion=15.0/20120820141800
RelatedPackageVersions:
 rhythmbox-mozilla 2.97-1ubuntu3
 google-talkplugin 3.2.4.0-1
 totem-mozilla 3.4.3-0ubuntu1
RunningIncompatibleAddons: False
SourcePackage: firefox
UpgradeStatus: Upgraded to quantal on 2012-08-07 (15 days ago)
dmi.bios.date: 03/25/10
dmi.bios.vendor: Apple Inc.
dmi.bios.version: MBP71.88Z.0039.B05.1003251322
dmi.board.name: Mac-F222BEC8
dmi.board.vendor: Apple Inc.
dmi.chassis.type: 10
dmi.chassis.vendor: Apple Inc.
dmi.chassis.version: Mac-F222BEC8
dmi.modalias: dmi:bvnAppleInc.:bvrMBP71.88Z.0039.B05.1003251322:bd03/25/10:svnAppleInc.:pnMacBookPro7,1:pvr1.0:rvnAppleInc.:rnMac-F222BEC8:rvr:cvnAppleInc.:ct10:cvrMac-F222BEC8:
dmi.product.name: MacBookPro7,1
dmi.product.version: 1.0
dmi.sys.vendor: Apple Inc.

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:12.0) Gecko/20100101 Firefox/12.0
Build ID: 20120424151726

Steps to reproduce:

It would be very convenient, for applications or addons interacting with the desktop or window manager, if nsIBaseWindow could expose its handle to the native window, in the form of an attribute in JS. This handle could be passed on to js-ctypes for instance.

It should be an number representing a pointer to the native window (like a *GdkWindow or a HWND). I guess it should be whatever nsWindow::GetNativeData(NS_NATIVE_WINDOW) returns but in JS;

This news thread shows a practical use case: https://groups.google.com/d/msg/mozilla.dev.extensions/JXgOCHSK0ZU/bR5A2ZCZV3sJ

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

I'd be happy to work on this one.

[Roc-ocallahan (roc-ocallahan)]

The basic idea sounds reasonable to me. You'll have to cope with the possibilty of this method returning null. That will certainly be true for all non-root windows.

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 632872
basic implementation

See attachment for a basic/proof-of-concept implementation: nativeHandle returns the actual address of the native window as a jsval (not a pointer as one could expecte).

However, I begin to have doubts about the usefulness of such a property:

1. getting a handle is relatively easy with ctypes. Extensions do that well already: see http://forums.mozillazine.org/viewtopic.php?f=19&t=2449775 (windows) or https://addons.mozilla.org/en/firefox/addon/firetray/ (linux).

2. I can't seem to be able to make use of the memory address provided by nativeHandle: I can build a ctypes object out of it (gdk.GdkWindow.ptr), but it does not behave properly when I try to manipulate it, for example when fetching the GtkWindow stored in the user_data of the GdkWindow.

Here is how I re-construct the ctypes window object:

  let baseWin = win.QueryInterface(Ci.nsIInterfaceRequestor)
    .getInterface(Ci.nsIWebNavigation)
    .QueryInterface(Ci.nsIDocShellTreeItem)
    .treeOwner
    .QueryInterface(Ci.nsIInterfaceRequestor)
    .nsIBaseWindow;
  let nativeHandle = baseWin.nativeHandle;
  let gdkwPtr = ctypes.uintptr_t(nativeHandle).address();
  let gdkWindow = ctypes.cast(gdkwPtr, gdk.GdkWindow.ptr);

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 635402
basic implementation as String instead of Number

chatting with Yoric, it turned out JS Numbers can only store 32 bits numbers (hence ctypes.UInt64!). So here is another basic implementation which returns a String that can easily be parsed by ctypes, like this:

      let nativeHandle = baseWin.nativeHandle;
      let gdkw = new gdk.GdkWindow.ptr(ctypes.UInt64(nativeHandle));

Note this version returns the protected pointer to the actual native window (HWND, GdkWindow*, ...), instead of the actual address of the window as previously implemented.

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

The problems mentioned in comment 3 point 2. are gone, and it's relatively easy to get the toplevel GtkWindow, with a gdk_window_get_user_data() followed by a gtk_widget_get_toplevel().

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 637885
Adds JS nativeHandler attribute for nsIBaseWindow

tested on Try: https://tbpl.mozilla.org/?tree=Try&rev=7f3510e83597 (I hope I didn't miss anything)

Few things to note:
* nativeHandle only implemented for nsXULWindow, not for embedding nsWebBrowser, not for nsDocShell (is this needed ?)
* if nsXULWindow->mWindow is unlikely to be void, shouldn't nativeHandle throw NS_ERROR_UNEXPECTED instead returning of JSVAL_NULL ? If not, how to test a nsXULWindow without a mWindow ?
* jsapi.h included in nsIBAseWindow.idl for jsval. It this the proper way ?
Thank you

[Roc-ocallahan (roc-ocallahan)]

Comment on attachment 637885
Adds JS nativeHandler attribute for nsIBaseWindow

Review of attachment 637885:
-----------------------------------------------------------------

::: widget/nsIBaseWindow.idl
@@ +163,5 @@
> +
> + @throws NS_ERROR_NOT_IMPLEMENTED for non-toplevel widgets, or
> + NS_ERROR_UNEXPECTED if conversion fails
> + */
> + [implicit_jscontext] readonly attribute jsval nativeHandle;

Why not declare it as a DOMString instead of a jsval?

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 638153
attribute as DOMString

There is no reason to declare the attribute as a jsval, as long as the string is available to JS. I guess DOMString is even better.

[Roc-ocallahan (roc-ocallahan)]

Comment on attachment 638153
attribute as DOMString

Review of attachment 638153:
-----------------------------------------------------------------

::: widget/nsIBaseWindow.idl
@@ +156,5 @@
> + This is the handle (HWND, GdkWindow*, ...) to the native window of the
> + control, exposed as a DOMString.
> +
> + @return DOMString in hex format with "0x" prepended, or null if mainWidget
> + undefined

It seems you don't actually return null, but an empty string if there is no native window.

It might be better to simply return an empty string for all failure conditions. I'm not sure it's worth trying to distinguish different failure conditions.

::: widget/tests/test_bug760802.html
@@ +57,5 @@
> + "nativeHandle should not be implemented for nsDocShell"
> +);
> +
> +SimpleTest.ok(typeof(nativeHandle) === "string", "nativeHandle should be a string");
> +SimpleTest.ok(nativeHandle.match(/^0x[0-9a-f]+/), "nativeHandle should have a memory address format");

You don't need to prefix ok() and is() with SimpleTest, do you?

Have you tested this on Windows? Visual C++ produces capital hex digits for %p. Maybe you should put a tolower method call somewhere so only lower-case digits are returned by this API.

::: xpfe/appshell/src/nsXULWindow.cpp
@@ +753,5 @@
> + |Number| for instance) */
> + char* addrStr = PR_smprintf("0x%p", nativeWindowPtr);
> + if (!addrStr)
> + return NS_ERROR_UNEXPECTED;
> + aNativeHandle = NS_ConvertASCIItoUTF16(addrStr);

You could use nsPrintfCString instead for slightly simpler code (and avoiding an error path).

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 638225
revised with nsPrintfCString

> It might be better to simply return an empty string for all failure conditions.

Do you mean it should not even throw NS_NOT_IMPLEMENTED for non-XULWindows (i.e. nsDocShell and nsWebBrowser) and return an empty string instead ?

> You don't need to prefix ok() and is() with SimpleTest, do you?

Changed applied.

> Have you tested this on Windows? Visual C++ produces capital hex digits for %p.
> Maybe you should put a tolower method call somewhere so only lower-case digits
are returned by this API.

I haven't tested on Windows myself, but my last Try (https://tbpl.mozilla.org/?tree=Try&rev=ad3559d462f6) and this test

  ok(nativeHandle.match(/^0x[0-9a-f]+$/), "nativeHandle should have a memory address format");

tend to show that a tolower method call is not useful. nsPrintfCString uses PR_vsnprintf, which converts using lowercase digits.

[Roc-ocallahan (roc-ocallahan)]

> > It might be better to simply return an empty string for all failure conditions.
>
> Do you mean it should not even throw NS_NOT_IMPLEMENTED for non-XULWindows
> (i.e. nsDocShell and nsWebBrowser) and return an empty string instead ?

Maybe. It doesn't really matter I guess. Leave it.

> tend to show that a tolower method call is not useful. nsPrintfCString uses
> PR_vsnprintf, which converts using lowercase digits.

OK!

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Comment on attachment 638225
revised with nsPrintfCString

roc, are you able to commit the patch as I don't have authorization yet ? Or should I add the checkin-needed keyword ? Is there anything I can do to help having the patch checked in ?

[Patrick Cloke (clokep)]

Drive by review comments:
- The spacing in embedding/browser/webBrowser/nsWebBrowser.cpp seems wrong (it seems to use 3 spaces, the added code uses 2).
- The comment in widget/nsIBaseWindow.idl has awful whitespace (some tabs, some two spaces).

Also, setting this to assigned...since it doesn't seem to be unconfirmed any longer.

[Roc-ocallahan (roc-ocallahan)]

Comment on attachment 638225
revised with nsPrintfCString

Review of attachment 638225:
-----------------------------------------------------------------

Please address those drive-by review comments, then reattach your patch with "checkin?" (no assignee) and someone will check it in :-)

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Created attachment 644612
with spacing fixes

Thank you Patrick for your careful review. I completely missed the spacing issue. It looks like some files have incorrect indentation and emacs/vim mode lines (see Bug 369322 for instance). I guess I'll need to file specific bug reports to correct some.

I also took the opportunity to remove this correction https://bugzilla.mozilla.org/attachment.cgi?id=638225&action=diff#a/testing/mochitest/static/xul.template.txt_sec1 which I believe belongs to a separate bug.

https://tbpl.mozilla.org/?tree=Try&rev=0cd412803918

[Foudil-newbie+bugzilla-mozilla-org (foudil-newbie+bugzilla-mozilla-org)]

Comment on attachment 644612
with spacing fixes

Re-reading Roc's comment 14, I removed the review? flag and set checkin?.

[Ryanvm (ryanvm)]

Comment on attachment 644612
with spacing fixes

You can just use checkin-needed. Also, please follow the directions below for future patches. It makes life much easier for those checking in on your behalf. Thanks!
https://developer.mozilla.org/en/Creating_a_patch_that_can_be_checked_in

[Ryanvm (ryanvm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9dcd7470dc0b

[Bmo-edmorley (bmo-edmorley)]

https://hg.mozilla.org/mozilla-central/rev/9dcd7470dc0b

[Chris Coulson (chrisccoulson)]

The patch breaks ABI, so we can't include it in its current form without making our builds ABI incompatible with upstream builds (which is bad for anybody installing binary addons, because they'll make our builds crash).

The new attribute will need splitting out in to a new interface if we want this functionality in the current Firefox version

[Alex Launi (alexlauni)]

Chris, that's what we've done in the other patch submitted to you for this same feature. A nativeHandleGetter interface is defined, and implemented for Gdk.

[Alex Launi (alexlauni)]

Also, that patch was submitted to mozilla but rejected due to the other one having already been accepted for FF17.
https://bugzilla.mozilla.org/show_bug.cgi?id=784791

[Ubuntu Foundations Team Bug Bot (crichton)]

The attachment "native-handle-getter.diff" of this bug report has been identified as being a patch. The ubuntu-reviewers team has been subscribed to the bug report so that they can review the patch. In the event that this is in fact not a patch you can resolve this situation by removing the tag 'patch' from the bug report and editing the attachment so that it is not flagged as a patch. Additionally, if you are member of the ubuntu-reviewers team please also unsubscribe the team from this bug report.

[This is an automated message performed by a Launchpad user owned by Brian Murray. Please contact him regarding any issues with the action taken in this bug report.]

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Chris Coulson (chrisccoulson)]

Alex - can you verify that the new beta works:

https://launchpad.net/~mozillateam/+archive/firefox-next

(you'll need to manually download the packages from Launchpad for now, until I turn publishing back on once I've smoke-tested builds for all of the other releases).

The patch I've committed is http://bazaar.launchpad.net/~mozillateam/firefox/firefox-beta.head/view/head:/debian/patches/add-nativehandle-attribute.patch. Functionally, it's the same as the one committed upstream, except you'll be using nsIBaseWindow_UBUNTU_ONLY.nativeHandle rather than nsIBaseWindow.nativeHandle, and the ABI of nsIBaseWindow is preserved. Note, the patch won't exist in Firefox 17, and so you'll need to support nsIBaseWindow.nativeHandler as well. Supporting both ways is trivial though (if ("nsIBaseWindow_UBUNTU_ONLY" in Components.interfaces) { <pre-17 code> } else { post-17 code })

[Launchpad Janitor (janitor)]

This bug was fixed in the package firefox - 15.0.1+build1-0ubuntu1

---------------
firefox (15.0.1+build1-0ubuntu1) quantal; urgency=low

  * New upstream stable release (FIREFOX_15_0_1_BUILD1)

  * Fix LP: #1040313 - expose scriptable native window handle
    - add debian/patches/add-nativehandle-attribute.patch
    - update debian/patches/series.in
 -- Chris Coulson <email address hidden> Thu, 06 Sep 2012 10:59:59 +0100

[Kate Stewart (kate.stewart)]

From IRC discussion, it seems more packages are affected. These should be added to the "Affects". Any dependencies on other bug fixes landing should also be spelled out explicitly.

[Ken VanDine (ken-vandine)]

The remaining packages to land are both new:

unity-firefox-extension lp:~webapps/unity-firefox-extension/quantal
webapps-greasemonkey lp:~webapps/webapps-greasemonkey/quantal

These have both been in ppa:webapps/preview since July

There is a related FFe requested for the default webapps included, bug 1046840

[Chris Coulson (chrisccoulson)]

If I understand http://bazaar.launchpad.net/~webapps/libunity-webapps/trunk/view/head:/src/webapps-service/unity-webapps-service.c correctly, this webapps stuff is dependent on multiple profile support in Firefox.

Just so you know, multiple profile support is being ripped out of Firefox. Here's the bugzilla patch that's currently being reviewed for it: https://bug214675.bugzilla.mozilla.org/attachment.cgi?id=416998

[Iain Lane (laney)]

To update the bug, we are currently discussing some difficult choices in the implementation between Chris Coulson and PS.

[Chris Coulson (chrisccoulson)]

It looks like you're symlinking folders / files between profiles too: http://bazaar.launchpad.net/~webapps/libunity-webapps/trunk/view/head:/src/webapps-service/unity-webapps-service.c#L827.

This is an accident waiting to happen, and is not really acceptable at all. Firefox prevents multiple processes from accessing the profile folder for good reason, and whilst you might get away with it for the cookies.sqlite (for now), this is likely to cause strange heisenbugs with the extensions folder which could manifest as extensions not loading or being marked as installed by a third party.

(extensions.ini will be replaced by a regular file on the first shutdown after any addons have chaned anyway)

How do you handle synchronization and locking between processes when the addon manager installs addon updates?

[Chris Coulson (chrisccoulson)]

Also, what do you do when the user removes the original profile, leaving the symlinks dangling? (this can be done by using the "Reset Firefox" feature available in Help -> Troubleshooting information).

This will delete cookies for all webapps, and leave the addon manager broken

[Roc-ocallahan (roc-ocallahan)]

Please make the test a chrome test

[Maxim Ermilov (zaspire)]

> multiple profile support is being ripped out of Firefox. Here's the bugzilla patch that's currently being reviewed for it
--profile <dir> will work.
>this is likely to cause strange heisenbugs with the extensions folder which could manifest as extensions not loading or being marked as installed by a third party.

what about copying this folder?

[Chris Coulson (chrisccoulson)]

Yes, -profile will still work, I'd forgotten about that. In that case, please use this then (which means you also don't need to touch profiles.ini, and you don't really need to have the profiles in ~/.mozilla/firefox). Although it's unfortunate that we've ended up with a solution that relies on the use of -no-remote, when the chromeless webapp runtime provided with Firefox doesn't depend on this...

As for copying the extensions folder - this would only work reliably if Firefox isn't running, wouldn't it? (else you could do the copy mid-update, or mid-install). But it's odd that in a "chromeless" mode that the addon manager chrome is still exposed via the menus anyway. With the upstream chromeless runtime, there really is no chrome at all (including addon manager). Because there is no addon manager, they've disabled addon support entirely in their webapp runtime (although we re-enable it in our Ubuntu package for system-wide addons in order to make language packs and the menubar work, and because system-wide addons don't depend on the addon manager chrome. But user-profile addons remain disabled).

[Robert Carr (robertcarr)]

Perhaps it is possible to use a default profile template for the chromeless?

I hate to say this because Maxim did great work on getting this Chromeless mode working. If it were earlier in the cycle, I would be inclined to say we could work out a solution.

Currently though we are RIGHT on our deadline :) Personally I don't even feel I have the mental space to fully evaluate the risk of the various options over the next 2 days.

Unless someone else is willing/confident to take a strong ownership on getting this sorted within the next 12-18 hours I am going to ask that we remove chromeless mode for quantal? Landing the firefox extension is really important!

[Olli Ries (ories)]

we are PAST any reasonable deadline. Drop it if this unblocks the release of the extension

[Robert Carr (robertcarr)]

Going to drop this code. Sorry for the trouble this late landing has caused everyone...update soon!

[Alvaro Lopez Ortega (alobbs)]

It is a pity. If the issue would have been raised earlier during the cycle we could have fixed it up on time though. <SIGH>

[Maxim Ermilov (zaspire)]

https://code.launchpad.net/~zaspire/libunity-webapps/chromeless-fix/+merge/125101

[Jamie Strandboge (jdstrand)]

FYI, bug #1043461 had some discussion regarding the maintenance of the browser extensions (particularly comments 12-14). If this is intended for main, please make sure the concerns discussed there are addressed.

[Robert Carr (robertcarr)]

After some discussion with Chris on IRC chromeless-fix branch has been reworked a little and merged, in a way which should be satisfying. Is there anything blocking this now?

[Robert Carr (robertcarr)]

Just wanted to clarify in response to Jamie (#43) that the discussion is ongoing at #1043461

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Michael Terry (mterry)]

I'm assuming the doc approval in bug 1046840 extends to these two plugins (unity, greasemonkey), as they are part and parcel of the same "Feature".

[Michael Terry (mterry)]

There are two new strings in these plugins.

[Michael Terry (mterry)]

Testing wise... greasemonkey has none yet. I've filed bug 1053621 to track that.

unity has a test suite that I've enabled via a dep8 test in my packaging branch.

[Michael Terry (mterry)]

Another comment from the dev team on one part of the unity extension (the ability to ask for the xid of a tab):
<lamalex> mterry, the xid patch has been approved by mozilla and is in firefox 17, our is just a backport for FF16 that preserves the current firefox 16 ABI so addons will continue to work
<lamalex> (and has been tested to ensure it does that)

[Kate Stewart (kate.stewart)]

I've asked mterry to upload the outstanding packages to -proposed, and will ask the release team experts to review for safety prior to copy over for beta 2.

[Michael Terry (mterry)]

Kate asked me to upload these to quantal-proposed for further review, so I did.

webapps-greasemonkey 2.3.1-0ubuntu1 and unity-firefox-extension 2.3-0ubuntu1 are in NEW now.

[Jamie Strandboge (jdstrand)]

I'll ACK this one for main. The maintenance discussion moved to bug #1040313.

[Chris Coulson (chrisccoulson)]

The change to libunity-webapps addresses my main concern (symlinks between profiles and dependency on the profile manager).

The chromeless mode feels a little unfinished though. Visually, the tab strip looks pretty weird without the navigation bar, probably because there is a horizontal line separating the tab strip and the content area (see http://ubuntuone.com/79PlBEH8uxo8dpMcXZ3OdQ). It also seems quite strange that a "chromeless" mode still exposes all browser chrome and browser features (with the exception of just the navigation bar) via the menubar. When I think of "chromeless", I think of a shell to view content without any browser chrome or browser-services running, eg, http://ubuntuone.com/1TE3gfs7D3vTzTouPCMzVe

Note that the View -> Tools -> Navigation Toolbar menu item is still displayed, but doesn't work.

What version of Greasemonkey is this based on? There was a recent greasemonkey update to fix a severe memory leak (https://bugzilla.mozilla.org/show_bug.cgi?id=778318). Does the version we're planning to ship have this fix?

[Maxim Ermilov (zaspire)]

> When I think of "chromeless", I think of a shell to view content without any browser chrome or browser-services running
It will look like this, when only one tab is opened.

> There was a recent greasemonkey update to fix a severe memory leak
that parts was removed from webapps-greasemonkey