1303798 - Beta mochitest(5) failing with test_ext_webrequest.html | Test timed out.

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=1608440&repo=mozilla-beta

https://archive.mozilla.org/pub/firefox/tinderbox-builds/mozilla-beta-win32-pgo/1474292431/mozilla-beta_win7_ix_test_pgo-mochitest-e10s-5-bm109-tests1-windows-build14.txt.gz

Comment 1

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Hi Giorgio, is this fallout from bug 1201979 / https://hg.mozilla.org/releases/mozilla-aurora/rev/3b04c5fb4e57

In the log, there is this:

09:34:47     INFO -  1249 INFO TEST-START | toolkit/components/extensions/test/mochitest/test_ext_webrequest.html
09:34:47     INFO -  1250 INFO SpawnTask.js | Entering test
09:34:47     INFO -  1251 INFO Extension loaded
09:34:47     INFO -  JavaScript error: moz-extension://1c4e5c7c-2080-4473-982b-e18a11e1e29b/%7B3b09dc87-a067-4618-af24-75dda4766505%7D.js, line 351: Error: Permission denied to access property "message"
09:39:56     INFO -  TEST-INFO | started process screenshot
09:39:56     INFO -  TEST-INFO | screenshot: exit 0
09:39:56     INFO -  1252 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest.html | Test timed out.

Comment 2

[Andrew Swan [:aswan]]

I think this feature was only enabled in aurora and nightly, so the test needs to be skipped on beta and release.
Kris did similar checks for webextension experiments like this:
http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_webextension.js#307-309

Comment 3

[Giorgio Maone [:ma1]]

What is happening at https://dxr.mozilla.org/mozilla-central/rev/f0f15b7c6aa77a0c5750918aa0a1cb3dc82185bc/toolkit/components/extensions/test/mochitest/test_ext_webrequest.html#446 is that the exception handler triggered by the expected lack of requestBody support in release builds unexpectedly throws, because the original exception's "message" property is not accessible for some reason ("permission denied", not sure why).

However, here we just wanted to check that the exception we are about to swallow is actually caused by requestBody not being supported, rather than any other unexpected cause which, on the other hand, seems very unlikely.

Therefore I believe just removing line 446 and 447 would be an acceptable fix.

Comment 4

[Intermittent Failures Robot]

18 automation job failures were associated with this bug yesterday.

Repository breakdown:
* mozilla-beta: 16
* mozilla-inbound: 1
* autoland: 1

Platform breakdown:
* windows7-32: 4
* osx-10-10: 4
* linux32: 4
* windowsxp: 2
* windows8-64: 2
* linux64: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1303798&startday=2016-09-19&endday=2016-09-19&tree=all

Comment 5

[Giorgio Maone [:ma1]]

Attachment: Bug 1303798 - Remove exception message check when requestBody is not supported.

Review commit: https://reviewboard.mozilla.org/r/79668/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/79668/

Comment 6

[Giorgio Maone [:ma1]]

Comment on attachment 8792764 [details]
Bug 1303798 - Remove exception message check when requestBody is not supported.

Approval Request Comment
[Feature/regressing bug #]: 1201979
[User impact if declined]: tests fail
[Describe test coverage new/current, TreeHerder]: try on central (don't know how to test in beta)
[Risks and why]: it may swallow an exception revealing requestBody tests were skipped
[String/UUID change made/needed]: none

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8792764 [details]
Bug 1303798 - Remove exception message check when requestBody is not supported.

https://reviewboard.mozilla.org/r/79668/#review78428

Hm. I think the correct fix is to emit an error that actually belongs to the calling scope. I'm worried that this change is going to lead to this test failing and no-one noticing it.

The way that we normally handle this is by throwing either a bare object, and converting that to an Error that belongs to the calling scope from the schema wrappers in ExtensionUtils.jsm[1], or throwing an Error crated in the extension's scope to begin with.

So, ideally, I think we should probably handle this by changing the other function call wrappers to handle error objects that the extension doesn't have access to[2], and create an `Error` subclass that WebRequest.jsm can throw, which `normalizeError` can check so that it knows to use the original error message, rather than replacing it with "An unexpected error occurred".

[1]: http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/toolkit/components/extensions/ExtensionUtils.jsm#336
[2]: http://searchfox.org/mozilla-central/rev/dbbbafc979a8362a1c8e3e99dbea188fc832b1c5/toolkit/components/extensions/ExtensionUtils.jsm#1577

Comment 8

[Kris Maglione [:kmag]]

In the short term, I'm just going to land a patch to beta that disables this call on non-release builds, so this stops failing. We can land a proper fix as a follow-up.

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8792764 [details]
Bug 1303798 - Remove exception message check when requestBody is not supported.

Test-only changes are auto-approved for uplift.

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-beta/rev/71b46b283225

Comment 11

[Carsten Book [:Tomcat]]

thanks kris and mao

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This still needs to land on trunk and Aurora unless I'm mistaken?

Comment 13

[Carsten Book [:Tomcat]]

kris, we need the same patch you checked-in on beta also on trunk and aurora or ? (could you attach the patch since its not the one the landed from this bug)

Comment 14

[Kris Maglione [:kmag]]

Aurora and central will need a longer term fix, as described in comment 7. The fix landed in beta is only a stopgap.

Comment 15

[Giorgio Maone [:ma1]]

However the "SpecialPowers is undefined" seems to be a real issue breaking requestBody file upload tests in non-release versions:


5929 07:51:31 INFO - ReferenceError: SpecialPowers is not defined
12040 08:04:19 INFO - 1538 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_ext_webrequest.html | Upload http://mochi.test:8888/tests/toolkit/components/extensions/test/mochitest/file_WebRequest_page3.html?trigger=form&upload=%7B%22%5C%22special%5C%22+ch%C3%83%C2%A0rs%22%3A%5B%22sp%C3%A2%E2%80%9A%C2%ACcial%22%5D%2C%22testFile%22%3A%5B%22testFile.pdf%22%5D%2C%22emptyFile%22%3A%5B%22%22%5D%2C%22textInput1%22%3A%5B%22value1%22%5D%7D&enctype=multipart%2Fform-data #22 matc

https://treeherder.mozilla.org/logviewer.html#?job_id=27806425&repo=try#L5929

Might be anything new about SpecialPowers in mochitests causing this?

Comment 16

[Kris Maglione [:kmag]]

The SpecialPowers error is a red herring. It looks like the problem is that the form data is coming through with broken encoding. Which is because of the patch for bug 1304331, which changes the encoding of the document to something unreasonable.

Comment 17

[Intermittent Failures Robot]

20 automation job failures were associated with this bug in the last 7 days.

Repository breakdown:
* mozilla-beta: 18
* mozilla-inbound: 1
* autoland: 1

Platform breakdown:
* windows8-64: 4
* windows7-32: 4
* osx-10-10: 4
* linux32: 4
* windowsxp: 2
* linux64: 2

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1303798&startday=2016-09-19&endday=2016-09-25&tree=all

Comment 18

[Carsten Book [:Tomcat]]

i guess we need to fix the problem still on aurora because when aurora becomes beta we might run into this as tree closing again

Comment 19

[Carsten Book [:Tomcat]]

[Tracking Requested - why for this release]:

aurora as beta uplift show perma failures/timeouts - so we need to fix this before merge day

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

Blocks moving 51 to beta; tracking.

Comment 21

[Kris Maglione [:kmag]]

This won't cause any problems when aurora becomes beta. The fix was for beta builds in general, not just the current beta.

Comment 22

[Kris Maglione [:kmag]]

Actually, no, I guess I'm wrong, since I didn't land the fix.

Comment 23

[Kris Maglione [:kmag]]

Attachment: Bug 1303798: Correctly propagate addListener errors from WebRequest.jsm to extension.

Review commit: https://reviewboard.mozilla.org/r/90938/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90938/

Comment 24

[Carsten Book [:Tomcat]]

(In reply to Kris Maglione [:kmag] from comment #22)
> Actually, no, I guess I'm wrong, since I didn't land the fix.

:) https://treeherder.mozilla.org/#/jobs?repo=try&revision=80b53a49a79266832c5af7cc99d6442b9e239fbf&selectedJob=30468193&filter-searchStr=mochitest%20m(5) is actually the simlation that show the failures

Comment 25

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8808018 [details]
Bug 1303798: Correctly propagate addListener errors from WebRequest.jsm to extension.

https://reviewboard.mozilla.org/r/90938/#review90866

Comment 26

[Shane Caraveo (:mixedpuppy)]

This is going to need a different patch for uplift unless we also uplift bug 1314493.

Comment 27

[Kris Maglione [:kmag]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> This is going to need a different patch for uplift unless we also uplift bug
> 1314493.

Yeah. Can you request uplift for that? It's probably a good idea, anyway.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/045ce2ca1b7d
Correctly propagate addListener errors from WebRequest.jsm to extension. r=mixedpuppy

Comment 29

[Kris Maglione [:kmag]]

Comment on attachment 8808018 [details]
Bug 1303798: Correctly propagate addListener errors from WebRequest.jsm to extension.

Approval Request Comment
[Feature/regressing bug #]: Bug 1201979
[User impact if declined]: This bug causes extensions to receive an Error object that they do not have privileges to access, which incidentally causes tests to fail when running in a beta or release build.
[Describe test coverage new/current, TreeHerder]: Existing tests detect this issue and confirm that it's fixed when running on beta.
[Risks and why]: Very low. This change primarily ensures that extensions can access an error object that they are intended to be able to access.
[String/UUID change made/needed]: None.

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=6245609&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/7e604b351f4faaa1f602c02dbae5639ce4d16b47

Comment 31

[Kris Maglione [:kmag]]

Turns out I tested with the RELEASE_OR_BETA flag set, but forgot to test without it...

Comment 32

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c2ad6366c6be9fce02428ca22b973be8d9d295b
Bug 1303798: Correctly propagate addListener errors from WebRequest.jsm to extension. r=mixedpuppy

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c2ad6366c6b

Comment 34

[Gerry Chang [:gchang]]

Comment on attachment 8808018 [details]
Bug 1303798: Correctly propagate addListener errors from WebRequest.jsm to extension.

Fix a mochitest error when running on beta. Take it in 51 aurora.

Comment 35

[Carsten Book [:Tomcat]]

needs rebasing for aurora

merging toolkit/components/extensions/ExtensionUtils.jsm
merging toolkit/components/extensions/test/mochitest/test_ext_webrequest_upload.html
merging toolkit/modules/addons/WebRequest.jsm
warning: conflicts while merging toolkit/components/extensions/ExtensionUtils.jsm! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 36

[Kris Maglione [:kmag]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/94d52b7d3d048a05f67f2c3c1736e4f1bdf5eee3