WebDriver:DeleteCookie throws TypeError: class constructors must be invoked with |new| when cookie does not exist

RESOLVED FIXED in Firefox 57

Status

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ato, Assigned: ato)

Tracking

(Blocks: 1 bug, {regression})

Version 3
mozilla58
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(URL)

Attachments

(6 attachments, 1 obsolete attachment)

(Assignee)

Description

2 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
(Assignee)

Updated

2 years ago
Assignee: nobody → ato
Blocks: 721859
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

2 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.
Attachment #8914790 - Flags: review?(mjzffr) → review-

Comment 16

2 years ago
mozreview-review
Comment on attachment 8914791 [details]
Bug 1405325 - Fix webdriver.transport.HTTPWireProtocol#url.

https://reviewboard.mozilla.org/r/186060/#review191892
Attachment #8914791 - Flags: review?(mjzffr) → review+

Comment 17

2 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
Attachment #8914792 - Flags: review?(mjzffr) → review+

Comment 18

2 years ago
mozreview-review
Comment on attachment 8914793 [details]
Bug 1405325 - Assign actual response to variable.

https://reviewboard.mozilla.org/r/186064/#review191898
Attachment #8914793 - Flags: review?(mjzffr) → review+

Comment 19

2 years ago
mozreview-review
Comment on attachment 8914794 [details]
Bug 1405325 - Correct HTTPWireProtocol#send documentation.

https://reviewboard.mozilla.org/r/186066/#review191902
Attachment #8914794 - Flags: review?(mjzffr) → review+

Comment 20

2 years ago
mozreview-review
Comment on attachment 8914795 [details]
Bug 1405325 - Align WebDriver:DeleteCookie with specification.

https://reviewboard.mozilla.org/r/186068/#review191904
Attachment #8914795 - Flags: review?(mjzffr) → review+

Comment 21

2 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?
Attachment #8914796 - Flags: review?(mjzffr) → review+
(Assignee)

Comment 22

2 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?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8914796 - Attachment is obsolete: true
(Assignee)

Comment 29

2 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.
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.
Blocks: 1376128
status-firefox56: --- → wontfix
status-firefox57: --- → affected
status-firefox58: --- → affected
Keywords: regression

Comment 31

2 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
Attachment #8914790 - Flags: review?(mjzffr) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

2 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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 51

2 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
(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?
Flags: needinfo?(ato)
(Assignee)

Comment 54

2 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.
Flags: needinfo?(ato)
Ok, I will leave it up to you then. My patch for fixing the new operator has already been uplifted to 57.
status-firefox-esr52: --- → unaffected
(Assignee)

Comment 56

2 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.
Whiteboard: [checkin-needed-beta]
You need to log in before you can comment on or make changes to this bug.