Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

When the WebDriver:DeleteCookie command is called with a cookie that
does not exist, it throws this error:

> A coding exception was thrown and uncaught in a Task.
> 
> Full message: TypeError: class constructors must be invoked with |new|
> Full stack: GeckoDriver.prototype.deleteCookie@chrome://marionette/content/driver.js:2620:9
> execute/req<@chrome://marionette/content/server.js:539:22
> TaskImpl_run@resource://gre/modules/Task.jsm:331:42
> TaskImpl@resource://gre/modules/Task.jsm:280:3
> asyncFunction@resource://gre/modules/Task.jsm:252:14
> Task_spawn@resource://gre/modules/Task.jsm:166:12
> execute@chrome://marionette/content/server.js:529:15
> onPacket@chrome://marionette/content/server.js:500:7
> _onJSONObjectReady/<@chrome://marionette/content/transport.js:501:9

This is because the following line is missing the "new" constructor
keyword:

>   throw UnknownError("Unable to find cookie");

This was originally reported in

	https://github.com/mozilla/geckodriver/issues/989

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 15 • 7 years ago
mozreview-review

Comment on attachment 8914790 [details]
Bug 1405325 - Rename wdspec cookies test to match command name.

https://reviewboard.mozilla.org/r/186058/#review191888

I believe it was previously decided upstream to keep all related tests in separate dirs. Therefore, please don't get rid of the cookies dir.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 16 • 7 years ago
mozreview-review

Comment on attachment 8914791 [details]
Bug 1405325 - Fix webdriver.transport.HTTPWireProtocol#url.

https://reviewboard.mozilla.org/r/186060/#review191892

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 7 years ago
mozreview-review

Comment on attachment 8914792 [details]
Bug 1405325 - Use HTTPWireProtocol#url to build full URL.

https://reviewboard.mozilla.org/r/186062/#review191894

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 18 • 7 years ago
mozreview-review

Comment on attachment 8914793 [details]
Bug 1405325 - Assign actual response to variable.

https://reviewboard.mozilla.org/r/186064/#review191898

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 19 • 7 years ago
mozreview-review

Comment on attachment 8914794 [details]
Bug 1405325 - Correct HTTPWireProtocol#send documentation.

https://reviewboard.mozilla.org/r/186066/#review191902

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 20 • 7 years ago
mozreview-review

Comment on attachment 8914795 [details]
Bug 1405325 - Align WebDriver:DeleteCookie with specification.

https://reviewboard.mozilla.org/r/186068/#review191904

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 21 • 7 years ago
mozreview-review

Comment on attachment 8914796 [details]
Bug 1405325 - Test deleting a cookie that does not exist.

https://reviewboard.mozilla.org/r/186070/#review191906

::: testing/web-platform/tests/webdriver/tests/delete_cookie.py:105
(Diff revision 2)
> +    create_dialog("confirm", text="dismiss #2", result_var="dismiss2")
> +
> +    response = delete_cookie(session, "foo")
> +    assert_error(response, "unexpected alert open")
> +    assert_dialog_handled(session, "dismiss #2")
> +
> +    create_dialog("prompt", text="dismiss #3", result_var="dismiss3")
> +
> +    response = delete_cookie(session, "foo")
> +    assert_error(response, "unexpected alert open")
> +    assert_dialog_handled(session, "dismiss #3")

I don't understand the difference between then second and third dialog checks. Are they both necessary?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 22 • 7 years ago
mozreview-review-reply

Comment on attachment 8914790 [details]
Bug 1405325 - Rename wdspec cookies test to match command name.

https://reviewboard.mozilla.org/r/186058/#review191888

I have moved the file.  But I don’t understand why you didn’t
give this an r+wc?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 29 • 7 years ago
mozreview-review-reply

Comment on attachment 8914796 [details]
Bug 1405325 - Test deleting a cookie that does not exist.

https://reviewboard.mozilla.org/r/186070/#review191906

> I don't understand the difference between then second and third dialog checks. Are they both necessary?

There’s one test for window.alert, one for window.confirm, and one
for window.prompt.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 30 • 7 years ago

This is a regression as introduced by the changes on bug 1376128. It means we cannot fix it for Firefox 56 because it has already been released, but we can do for both 57.0 and 58.0.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8914790 [details]
Bug 1405325 - Rename wdspec cookies test to match command name.

https://reviewboard.mozilla.org/r/186058/#review191992

	Mozilla Autoland
	Comment 38 • 7 years ago

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 8ed74cafa7c6 -d dfc766f28aec: rebasing 424757:8ed74cafa7c6 "Bug 1405325 - Rename wdspec cookies test to match command name. r=maja_zf"
merging testing/web-platform/meta/MANIFEST.json
warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

	Pulsebot
	Comment 51 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e942c22c4d85
Rename wdspec cookies test to match command name. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/18dd26f61c50
Fix webdriver.transport.HTTPWireProtocol#url. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/073cb0e1fd41
Use HTTPWireProtocol#url to build full URL. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/e5c5edd62184
Assign actual response to variable. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5a2f8087260d
Correct HTTPWireProtocol#send documentation. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/ff181baf62e0
Align WebDriver:DeleteCookie with specification. r=maja_zf

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 52 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e942c22c4d85
https://hg.mozilla.org/mozilla-central/rev/18dd26f61c50
https://hg.mozilla.org/mozilla-central/rev/073cb0e1fd41
https://hg.mozilla.org/mozilla-central/rev/e5c5edd62184
https://hg.mozilla.org/mozilla-central/rev/5a2f8087260d
https://hg.mozilla.org/mozilla-central/rev/ff181baf62e0

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 53 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #30)
> This is a regression as introduced by the changes on bug 1376128. It means
> we cannot fix it for Firefox 56 because it has already been released, but we
> can do for both 57.0 and 58.0.

The regression I'm referring here got fixed via bug 1406150. So not sure if the regression keyword is still appropriate. Andreas, would the cookie changes need an uplift to beta?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 54 • 7 years ago

(In reply to Henrik Skupin (:whimboo) from comment #53)

> (In reply to Henrik Skupin (:whimboo) from comment #30)
> 
> > This is a regression as introduced by the changes on bug
> > 1376128. It means we cannot fix it for Firefox 56 because it has
> > already been released, but we can do for both 57.0 and 58.0.
> 
> The regression I'm referring here got fixed via bug 1406150. So
> not sure if the regression keyword is still appropriate. Andreas,
> would the cookie changes need an uplift to beta?

I would characterise this as a regression from the previous
behaviour in Firefox < 56.  However, that behaviour was also wrong
and what changed with https://bugzil.la/1376128 was that it started
returning a WebDriverError (serialised JS TypeError) instead of an
UnknownError.

This changeset, in addition to fixing the TypeError, also changes
the _behaviour_ of WebDriver:DeleteCookie so that it no longer
returns an error.  Seeing as the previous behaviour was wrong
anyway, the question is if we care about fixing the TypeError in
Firefox 57?

There are two options as I see it: either we backport a fix for
the missing "new" constructor for UnknownError, or we uplift this
changeset and change the behaviour.  I would put uplifting this at
moderate risk, mostly due to WPT changes.

	Henrik Skupin [:whimboo][⌚️UTC+1]
	Comment 55 • 7 years ago

Ok, I will leave it up to you then. My patch for fixing the new operator has already been uplifted to 57.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 56 • 7 years ago

Sheriffs: Please (try to) uplift to beta.  If the changeset fails to
apply we need to handcraft a patch either fixing the "new" operator
or backport the behaviour.

	Ryan VanderMeulen [:RyanVM]
	Comment 57 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/b15dc7b3b413
https://hg.mozilla.org/releases/mozilla-beta/rev/62b8da629553
https://hg.mozilla.org/releases/mozilla-beta/rev/b817f99334fa
https://hg.mozilla.org/releases/mozilla-beta/rev/d06884758cb6
https://hg.mozilla.org/releases/mozilla-beta/rev/bd809f9f27e0
https://hg.mozilla.org/releases/mozilla-beta/rev/9a1314063088