Created attachment 211607 [details] [review]
also consider email inputs when looking for user/pass in forms

This does not fix the problem for monster, it seems, but does fix it for the cases I found (Amazon AWS and Udacity).

Review of attachment 211607 [details] [review]:

Looks good, missed the patch somehow before, sorry!

Comment on attachment 211607 [details] [review]
also consider email inputs when looking for user/pass in forms

No rush =). What do you think of this going onto the 3.4 branch?

Pushed to master as f9e00946d51a745f392ba88bf879e77bb157e663

I'm keeping the bug open because there are a few more cases to check.

(In reply to comment #3)
> (From update of attachment 211607 [details] [review])
> No rush =). What do you think of this going onto the 3.4 branch?
> 
> Pushed to master as f9e00946d51a745f392ba88bf879e77bb157e663

Sure, looks fine for gnome-3-4 too.

OK, pushed to gnome-3-4 too (at last!). 0a82c14c21cea3c6e1ce64fe291832b1f7e4769c

We still don't prompt for a password on bugzilla.gnome.org as of Epiphany 3.8.0.

There's been a significant regression in git master in this area.  As of revision 09f9b5f9, Epiphany no longer prompts to save my password when I log into sites including Facebook, Launchpad and Yorba.  Epiphany 3.8.0 does offer to save my password on all these sites.

(In reply to comment #8)
> There's been a significant regression in git master in this area.  As of
> revision 09f9b5f9, Epiphany no longer prompts to save my password when I log
> into sites including Facebook, Launchpad and Yorba.  Epiphany 3.8.0 does offer
> to save my password on all these sites.

I think it's impossible that this commit can break those things. Have you actually bisected it? Does the previous commit in master work?

I wasn't saying that that revision itself broke things - all I meant to say was that as of that revision (i.e. now) they are broken, but in 3.8.0 they were working.  I haven't bisected or otherwise looked at what happened in between.  Sorry for the confusion.

I can't reproduce it

OK - here's a clue.  On my machine, if I run Epiphany 3.8.0 and use pmap to look at WebKitWebProcess, I see that libephywebextension.so is present.  If I repeat using Epiphany from git master, libephywebextension.so is not listed.  So it appears that the web extension shared library isn't even being loaded.

How can I debug this further?  What mechanism would normally cause that library to load?

(In reply to comment #12)
> OK - here's a clue.  On my machine, if I run Epiphany 3.8.0 and use pmap to
> look at WebKitWebProcess, I see that libephywebextension.so is present.  If I
> repeat using Epiphany from git master, libephywebextension.so is not listed. 
> So it appears that the web extension shared library isn't even being loaded.
> 
> How can I debug this further?  What mechanism would normally cause that library
> to load?

Are you doing "make install" after compiling epiphany from git master? You need to do it for the extension to be picked up.

Certainly, yes.

(In reply to comment #14)
> Certainly, yes.

You can then break around ephy-shell.c:ephy_shell_startup and ephy-embed-shell.c:ephy_embed_shell_watch_web_extension and see what directory is being used to find extensions, whether you actually have anything there, and why exactly things are failing.

OK - I investigated.  The problem seems to be that on my machine, Epiphany creates the WebKitWebProcess *before* it calls webkit_web_context_set_web_extensions_directory.  So by the time it tries to set the directory it's too late: the WebProcess has already started and the injected bundle doesn't know where to look for web extensions.

The WebKitWebProcess is created at this stack trace:

WebKit::WebContext::createNewWebProcess
WebKit::WebContext::ensureSharedWebProcess
WebKit::WebContext::sendToNetworkingProcessRelaunchingIfNecessary
WebKit::WebCookieManagerProxy::startObservingCookieChanges
webkitCookieManagerCreate
webkit_web_context_get_cookie_manager
ephy_embed_shell_init
g_type_create_instance
g_object_constructor
g_object_newv
g_object_new_valist
g_object_new
_ephy_shell_create_instance
main

And set_web_extensions_directory is called at this stack trace:

webkit_web_context_set_web_extensions_directory
ephy_shell_startup
_g_closure_invoke_va
g_signal_emit_valist
g_signal_emit
g_application_register
g_application_real_local_command_line
g_application_real_local_command_line
g_application_run
main

main calls _ephy_shell_create_instance before g_application_run, so the first event above happens before the second.

So the question is, then: why does this not happen on your (Xan's and Carlos's) machines?

One more point: I'm running WebKitGtk 1.11.92 built from that release tarball, not from svn trunk.  Are you guys running WebKitGtk built from trunk and, if so, might that make a difference?  When I run Epiphany from master, do you generally recommend building WebKitGtk from the last development release tarball or from trunk?  Or should either generally be fine?

I just tried WebKitGTK from svn trunk to see if it would work better.  But actually I can't even build Epiphany with that version of WebKitGTK:

  CC     libephywidgets_la-ephy-hosts-store.lo
In file included from ephy-hosts-store.c:24:0:
../../embed/ephy-embed-prefs.h:39:1: error: unknown type name 'WebKitWebViewGroup'
make[5]: *** [libephywidgets_la-ephy-hosts-store.lo] Error 1

This might be due to bug #696020, I've just pushed a fix to git master, can you try again please?

Yes, it's working now!  The web extension is getting loaded into WebKitWebProcess and Epiphany is once again prompting to save passwords on most sites.  That's great.

There are still some sites where Epiphany fails to prompt (ironically, bugzilla.gnome.org is one example), so presumably we should still keep this bug open.

The ephy mebed single removal in master introduced (or revealed) this bug, that's why it worked in the stable branch.

(In reply to comment #19)
> There are still some sites where Epiphany fails to prompt (ironically,
> bugzilla.gnome.org is one example), so presumably we should still keep this bug
> open.

Do those other sites worked with webkit1? I think it's might be related to the code that detects forms, rather than the web extension not being loaded.

Those sites didn't work with webkit1 either.  I agree that the failure to prompt on those sites is probably related to the form detecting code.

This is a problem in in 3.8.2 for me, but I have not used epiphany previously, so don't know if I'm just testing it with websites that don't work for some unrelated reason…

The code to detect when there are forms with username/passwords pairs that could be saved is pretty simple and it can neglect some cases. There is bug 703870, for instance, and bug 698866, too. We probably need to extend the code to cover those cases and others.

*** Bug 724812 has been marked as a duplicate of this bug. ***

Ephy 3.16.0 and WKGTK 2.8.0

This doesn't seem to work with Tiny Tiny RSS login form.

I have a private installation and the dialog to remember user and password never shows up.

This can be tested with the demo at:
http://tinyrss.ru/

Login: test-tanty
Password: d3z96r8Q

Did this patch ever get committed? Bugzilla installations still not working, nor is login.ubuntu.com (type=email + type=password fields) as of 3.18.0.

I've looked at what Epiphany does differently from Firefox here, and it seems that we have to:

1. Store formSubmitURL property for the passwords, like Firefox does
1.1. When syncing, just get what comes with the synced records
1.2. When creating a new record, take the form's "action" attr value (if it's empty, just use the page URL) and take the protocol://hostname part of it.
2. Use formSubmitURL to find the correct form to fill. For each form, process it the same way as described above and compare result with formSubmitURL value
3. Accept forms with [1;3] password fields. If there are multiple password fields, just use the first one
4. Only check username fields *before* the first password field, not all of them
5. Currently, a username field is a field with types: "text", "email", "tel". Add to that "url" and "number". That shouldn't make a huge difference, but still
6. Possibly, support Firefox's site recipe quirks, see https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/passwordmgr/content/recipes.json

Reference:
toolkit/components/passwordmgr/LoginManagerContent.jsm, 
toolkit/components/passwordmgr/LoginHelper.jsm, 
toolkit/components/passwordmgr/LoginRecipes.jsm files in Firefox source

The list may be incomplete. Epiphany has a special case for fields that have name="adminpw" password fields, and no username field. Firefox doesn't seem to do this, and doesn't allow username-less records as well.

Addition concerning password fields.
Firefox accepts forms with 1, 2 or 3 password fields.
This is used to accomodate cases like:

Password: ...
Confirm password: ...
---------------------
Old password: ...
New password: ...
---------------------
Old password: ...
New password: ...
Confirm new password: ...

Firefox tries to grab both old and new passwords when submitting.
I have no intention to return both of these, but we can use them for some better heuristics.
Still, we have to distinguish them in these cases to avoid
grabbing old password in second and third cases above.

The logic is as follows:
1. We have 1 password field. Just use it as new password
2. We have 2 fields.
  1. If values are same, this is a "pw/confirm pw" case. Pick the first field as new password
  2. Values are different, this is "old pw/new pw" case. The first one is old, the second is new
3. We have 3 fields.
  1. All values are same. A weird case. Just pick the first field
  2. First and second values are same, the third is different. This is "new/confirm new/old" case. Pick the first and the last fields
  3. Second and third are same, the first is different. "old/new/confirm new" case, pick the last and the first fields
  4. First and third are same, the second is different. Another weird case. Pick first field as new, and second as old
  5. All values are different. Panic and ignore the form altogether

When loading, Firefox doesn't do that and just fills the first password field.
This is usually good, but will fail in cases 3.2 and 3.4, because it will fill "new password" field instead of "old password".

So, my idea is to:
1. Distinguish "user pressed submit" and "page loading/a field lost focus" cases, like Firefox does
2. Try to find both old and new password fields
3. If we are submitting, use new password field. The old password isn't relevant here.
4. If we are autofilling, check if we have found old password field. If we have, then it's a "change password" form and we have to fill the old password field. If we haven't, then it's a regular "login/password" form, so we just fill the new password field.

Another thing is that Firefox never rejects forms with no username fields. This addresses my concern about "adminpw" case.

Reference:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/passwordmgr/LoginManagerContent.jsm
Lines 800-855

An additional problem, aside of heuristic, is that some forms don't emit "submit" event for some reason.

(In reply to Mikhaylenko Alexander from comment #30)
> An additional problem, aside of heuristic, is that some forms don't emit
> "submit" event for some reason.

See bug #742573

Created attachment 358363 [details] [review]
sync: Store formSubmitURL for password records

Created attachment 358364 [details] [review]
profile-migrator: Add formSubmitURL to passwords

Adds formSubmitURL to existing password records,
with the same value as hostname.
Bumps profile migration version to 21.

Created attachment 358365 [details] [review]
sync: Prevent duplicates after formSubmitURL migration

If formSubmitURL value is different from hostname, then
migration and syncing will cause duplicate records. Add
a special case to prevent that

Created attachment 358366 [details] [review]
web-extension: Make usage of formSubmitURL

Get formSubmitURL value from forms and store it in keyring;
Only autofill passwords when formSubmitURL matches

Created attachment 358367 [details] [review]
web-extension: Implement Firefox form heuristic

(To everyone watching this bug: note that bug #703870 and bug #742573 are both still open. These patches are a big step in the right direction, though. Actually, I wonder if the last patch here fixes bug #742573.)

Review of attachment 358363 [details] [review]:

I would squash "Make usage of formSubmitURL" into this patch. The way you have it split into multiple patches is confusing, because you're not using formSubmitURL at all in this patch.

I think I might name it "formTargetURL" instead, but either way is fine.

I'm going to mark these as accepted, but it'd be great for Gabriel to review these, too, since he designed EphyPasswordManager.

::: lib/sync/ephy-password-manager.c
@@ +202,3 @@
                          g_strdup (HOSTNAME_KEY),
                          ephy_uri_to_security_origin (uri));
+  if (form_submit_url)

There should probably be a blank line above this if statement.

Review of attachment 358364 [details] [review]:

Looks good.

::: lib/ephy-profile-utils.h
@@ +29,3 @@
 #define EPHY_SETTINGS_MIGRATION_VERSION 16
 #define EPHY_FIREFOX_SYNC_PASSWORDS_MIGRATION_VERSION 19
+#define EPHY_FORM_SUBMIT_URL_MIGRATION_VERSION 21

Sigh. Eventually we will need to find a less-hacky way to run these global migrations.

Review of attachment 358365 [details] [review]:

I think we don't need this patch. It's only important for users who have been beta testing 3.25 already and so already had synced passwords originally created by Firefox, right? And then it's only to clean up the keyring a little, right? I'd rather just not add code to handle this. Good attention to detail, though!

> I would squash "Make usage of formSubmitURL" into this patch. The way you have it split into multiple patches is confusing, because you're not using formSubmitURL at all in this patch.
Ok.

> I think I might name it "formTargetURL" instead, but either way is fine.
The current way is consistent with property name in sync and keystore, but if you want to change it, no problem.

> There should probably be a blank line above this if statement.
If I add a blank line, it'll be inconsistent with the rest of the function:
https://github.com/GNOME/epiphany/blob/master/lib/sync/ephy-password-manager.c#L184

> It's only important for users who have been beta testing 3.25 already
No, why? If you update from 3.24, all your records have potentially incorrect formSubmitURL (and different IDs obviously), so if you just update from 3.24 and set up sync (and you had the same websites on the server side), then you get duplicates for those records.

Review of attachment 358366 [details] [review]:

::: embed/web-extension/ephy-web-extension.c
@@ +291,3 @@
   uri_str = soup_uri_to_string (uri, FALSE);
+  form_submit_url = ephy_embed_form_auth_get_form_submit_url (form_auth);
+  form_submit_url_str = soup_uri_to_string (form_submit_url, FALSE);

I think it would be easier to use ephy_embed_form_auth_get_form_submit_url() if it returned a char * rather than a SoupURL, since both times you use it, you just create a string with it right away anyway.

@@ +1143,3 @@
       const char *uri;
+      const char *form_action;
+      const char *form_submit_url;

Watch out for memory leaks!

char *form_action;
char *form_submit_url;

@@ +1147,3 @@
+      uri = webkit_web_page_get_uri (web_page);
+
+      form_action = webkit_dom_html_form_element_get_action (form);

It returns a gchar *, not a const gchar *, so you need to free it with g_free().

@@ +1150,3 @@
+      if (form_action == NULL)
+        form_action = uri;
+      form_submit_url = ephy_uri_to_security_origin (form_action);

OK, here is what I missed before. The parameter is named form_submit_url, but it's not a URL that you're putting into it. It's a security origin: a triplet of (protocol, host, port). So that's not right. Well, it's fine to store that, but we *really* need to rename the parameter to avoid confusing future developers (like me ;) as to how it's supposed to be used. So everywhere you have form_submit_url needs to be renamed. There are several possible names you can use. One option I like would be "target_origin". Normally in WebKit we use the terminology "target" for the form submission target, and "origin" makes it clear that it is a security origin. You could find a more verbose variant of that if you want to avoid confusion. But 

Also: ephy_uri_to_security_origin returns a char *, not a const char *, so you need to free it with g_free().

Review of attachment 358367 [details] [review]:

Great work. Thanks very much.

::: embed/web-extension/ephy-web-dom-utils.c
@@ +511,3 @@
+    g_object_get (element, "type", &element_type, "value", &element_value, NULL);
+    if (g_strcmp0 (element_type, "password") != 0)
+      continue;

Nope, you've just leaked element_type and element_value! The cleanest way to handle this is with goto, unfortunately. (In Epiphany we use goto only for cleaning up resources: freeing memory.) So instead of continue, you would write "goto next" and put a next label just above where you free them down below.

@@ +515,3 @@
+    if ((element_value == NULL || strlen (element_value) == 0) &&
+        skip_empty_fields)
+      continue;

Ditto.

@@ +527,3 @@
+  g_object_unref (elements);
+
+  /* We only want to process forms with 1-3 fields */

Why? I know it's because that's what Firefox does, but future developers reading this code won't know that. It's worth explaining your strategy in a comment.

@@ +594,1 @@
+    if (password_nodes->len == 2) {

E.g. above here would be a good place for a comment to say you took the strategy from Firefox. I would also mention which source code file, to make it easier to find in the future if needed. (Eventually, it will be needed....)

@@ +596,3 @@
+        /* Password / Confirm password */
+        password_node_index = 0;
+      else

Even though they're not needed here, it would help readability to add braces, since the blocks span multiple lines. And below as well.

@@ +635,3 @@
+  g_ptr_array_free (password_nodes, TRUE);
+
+  if (*username && *password)

Is this right? What about password forms with no username, like mailman passwords? E.g. https://mail.gnome.org/mailman/admindb/epiphany-list?

::: embed/web-extension/ephy-web-dom-utils.h
@@ +35,3 @@
 
+GPtrArray *ephy_web_dom_utils_find_password_fields (WebKitDOMHTMLFormElement *form,
+                                                    gboolean                  skip_empty_fields);

I don't like using a gboolean here. It's easy, but it makes the call site harder to read. (What is TRUE? What is FALSE? You can't tell without looking at the function declaration.) Instead, add an enum with two descriptive values to make it easier to read.

Boolean parameters are generally an antipattern, unless you're calling a setter function for a boolean property.

@@ +40,3 @@
                                                      WebKitDOMNode           **username,
+                                                     WebKitDOMNode           **password,
+                                                     gboolean                  is_submission);

Ditto.

Comment on attachment 358365 [details] [review]
sync: Prevent duplicates after formSubmitURL migration

exalm_	mcatanzaro: Ok. Agree with everything from the last 2 reviews, but I don't agree about the sync conditional. It's there precisely for 3.24->3.26 update case :)
Say, you use Firefox and Ephy 3.24. You have sync set up in Firefox. You have a website with formSubmitURL≠url
So, on Firefox (and on sync server) you have a record with correct formSubmitURL value
Also, you have the same website in Ephy, without formSubmitURL
Then, you update Ephy to 3.26. Migrator automatically adds a formSubmitURL=url
Then, you set up sync and get duplicate, because formSubmitURL is different
mcatanzaro	exalm_: Hm, OK then :/

(In reply to Alexander Mikhaylenko from comment #41)
> > I think I might name it "formTargetURL" instead, but either way is fine.
> The current way is consistent with property name in sync and keystore, but
> if you want to change it, no problem.

Hm, unfortunate that I missed this earlier. Gabriel, is it supposed to be a URL or a security origin? See comment #42.

> > There should probably be a blank line above this if statement.
> If I add a blank line, it'll be inconsistent with the rest of the function:
> https://github.com/GNOME/epiphany/blob/master/lib/sync/ephy-password-manager.
> c#L184

OK

> > It's only important for users who have been beta testing 3.25 already
> No, why? If you update from 3.24, all your records have potentially
> incorrect formSubmitURL (and different IDs obviously), so if you just update
> from 3.24 and set up sync (and you had the same websites on the server
> side), then you get duplicates for those records.

OK

Comment 42, comment 43: Agree with everything.

> Is this right?
It isn't right, I've missed that.

----------------------------------------------

Firefox calls its property formSubmitURL, but really
stores an origin there, so we decided to rename
form_submit_url to target_origin or form_target_origin everywhere
in the patchset, including in keyring schema, and add a reference
to this line in Mozilla Toolkit code:
https://github.com/mozilla/gecko-dev/blob/master/toolkit/components/passwordmgr/LoginManagerContent.jsm#L928
to make it clear why it's called formSubmitURL in the sync code.

Created attachment 358432 [details] [review]
sync: Store target_origin for password records

Since it matches formSubmitURL from Firefox sync spec,
rename form_submit_url field in EphyPasswordRecord

Created attachment 358433 [details] [review]
profile-migrator: Add target_origin to passwords

Adds target_origin to existing password records,
with the same value as hostname.
Bumps profile migration version to 21.

Created attachment 358434 [details] [review]
sync: Prevent duplicates after target_origin migration

If target_origin value is different from hostname, then
migration and syncing will cause duplicate records. Add
a special case to prevent that

Created attachment 358435 [details] [review]
web-extension: Implement Firefox form heuristic

Review of attachment 358435 [details] [review]:

::: embed/web-extension/ephy-web-dom-utils.c
@@ +664,3 @@
   }
 
+  *username = WEBKIT_DOM_NODE (username_node);

username_node can be NULL here, right? This still doesn't look safe. Telling is that there is an if (username_node) check just down below. :)

Created attachment 358440 [details] [review]
web-extension: Implement Firefox form heuristic

Comment on attachment 358440 [details] [review]
web-extension: Implement Firefox form heuristic

It doesn't actually print anything for me, but added a check nevertheless

Note that the dependent bugs are not yet fixed, so this problem is not completely solved, but this is a big step in the right direction.

Attachment 358432 [details] pushed as 809b3e4 - sync: Store target_origin for password records
Attachment 358433 [details] pushed as 46a2ed8 - profile-migrator: Add target_origin to passwords
Attachment 358434 [details] pushed as 4f54f52 - sync: Prevent duplicates after target_origin migration
Attachment 358440 [details] pushed as 93cb509 - web-extension: Implement Firefox form heuristic

In particular, bug #742573 is still a problem. To fix this we need to add new API in WebKit. I'm working on that, but it means it can't be fixed in Epiphany until 3.28.

*** Bug 774258 has been marked as a duplicate of this bug. ***