Closed Bug 1525319 Opened 4 years ago Closed 4 years ago

Investigate if we can remove Context argument from Channel methods

Categories

(Core :: DOM: Core & HTML, enhancement, P3)

Product:
Core
Component:
DOM: Core & HTML
Version:
65 Branch
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla67
Tracking Flags:
Tracking Status
firefox67 --- fixed

People

(Reporter: jkt, Assigned: jkt)

References

Details

Attachments

(4 files)

Bug 1525319 - Removing context from OnStartRequest
47 bytes, text/x-phabricator-request
Details | Review
Bug 1525319 - Removing context from OnStopRequest
47 bytes, text/x-phabricator-request
Details | Review
Bug 1525319 - Changing js to remove context from onStartRequest and onStopRequest
47 bytes, text/x-phabricator-request
Details | Review
Bug 1525319 - Removing context from OnDataAvailable
47 bytes, text/x-phabricator-request
Details | Review

With the move from AsyncOpen and Open having a Context argument we should investigate if it can also be removed from OnProgress, OnStartRequest, OnStopRequest, AsyncConvertData etc

This would largely simplify code auditing given that these arguments aren't consistently passed through the code base or just dropped within some class implementations.

Example https://searchfox.org/mozilla-central/rev/9eb30227b21e0aa40d51d9f9b08bb0b113c5fadb/dom/plugins/base/nsPluginStreamListenerPeer.cpp#357 sPluginStreamListenerPeer::OnStopRequest( doesn't use the argument however the other implementation of this method passes it.
Other cases like TRR just pass nullptr around as it's unused, it appears from all I have seen that it was only needed when the Open methods had a context.

Assignee: nobody → jkt

Hey Both,

Minus the windows fail to build and the ESLint fails that I have since fixed, it looks like this is all possible: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ad4f6654a60200e7e27c885dc6e9e64933c5275

What do you both think, this seems totally possible and with the removal from OnDataReceived too I think the mContext member can be removed.

Kind regards
Jonathan

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(dd.mozilla)

(In reply to Jonathan Kingston [:jkt] from comment #4)

What do you both think, this seems totally possible and with the removal from OnDataReceived too I think the mContext member can be removed.

Sounds good to me. 👍

Flags: needinfo?(valentin.gosu)

Ok I have managed to get a clean test run on OnStartRequest and OnEndRequest. I think soon as I get one for OnDataAvailable we should try and land due to how fast this will bitrot.

There will be a follow up task to remove it from other functions and also remove mContext from the classes themselves but I think that is much smaller anyway.

Flags: needinfo?(dd.mozilla)
Blocks: 1530209

Great job with this bug! Those are some huge patches 😊

Thanks for the review! I'll land straight away before they bit-rot, hopefully we caught it all. Thanks again

Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84ca79bd2dc3
Removing context from OnStartRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/6d73418988d4
Removing context from OnStopRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/1d318d5c6b98
Changing js to remove context from onStartRequest and onStopRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/b73f033efb41
Removing context from OnDataAvailable r=valentin

The error appears to be with:

*** error running SJS at /builds/worker/workspace/build/tests/mochitest/tests/dom/base/test/send_gzip_content.sjs: [Exception... "Not enough arguments [nsIStreamListener.onDataAvailable]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: file:///builds/worker/workspace/build/tests/mochitest/tests/dom/base/test/send_gzip_content.sjs :: gzipCompressString :: line 14" data: no] on line -2516

I have tried fixing this with try by touching the CLOBBER file, changing the uuid of the interfaces I am trying the NetCID. It appears that sjs xpcshell for Android isn't being updated. Failing the NetCID change I think I will disable the test for Android as it's clearly working else it wouldn't compile and I'm pretty sure sjs isn't critical path to getting a working Android.

I spent several hours trying to hunt down anything else that might be the cause too and there really isn't anything that would change the argument number for this interface.

Final try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7609bf3eb53a63e1e5019bb33a194e5fe17ce27

Flags: needinfo?(jkt)

Maybe you will have to update host utilities?
https://wiki.mozilla.org/Packaging_Android_host_utilities

https://bugzilla.mozilla.org/show_bug.cgi?id=1525319#c13 does sound like it is the issue, do I just request release engineering to make this change as part of the patch?

Reissuing a uuid and a clobber didn't help anyway.

Flags: needinfo?(valentin.gosu)
Flags: needinfo?(VYV03354)

I think so. Geoff, can you help with updating hostutils?

Flags: needinfo?(valentin.gosu) → needinfo?(gbrown)

:aerickson is taking care of host-utils updates now.

Flags: needinfo?(gbrown) → needinfo?(aerickson)
Flags: needinfo?(VYV03354)
Depends on: 1530546

OK, I'll update hostutils. I've created https://bugzilla.mozilla.org/show_bug.cgi?id=1530546 for tracking.

Flags: needinfo?(aerickson)

Does this happen before this patch lands or after? Sorry I'm not familiar with the build process here.

Flags: needinfo?(aerickson)

The version of hostutils used for builds/tests is stored in manifest files inside the repo. We build a new version, make a review with the new versions, test it, then land it. Once landed, you should rebase and then your code will use the new version of hostutils.

Flags: needinfo?(aerickson)

I perhaps don't understand enough about host utils but I would expect them to break in a different way if they have a different function signature that the current mozilla-central. Are you building Bug 1530546 with this patch included?

Flags: needinfo?(aerickson)

Sorry, I was confused (I thought this was a simple update request).

I need all 4 patches?

Flags: needinfo?(aerickson) → needinfo?(jkt)

Ideally however the only notable issue I saw was with the OnDataAvailable patch (which changes the number of call arguments). Perhaps the other methods don't have test coverage.

The failing test was dom/base/test/test_progress_events_for_gzip_data.html and the error that is causing it is:

*** error running SJS at /builds/worker/workspace/build/tests/mochitest/tests/dom/base/test/send_gzip_content.sjs: [Exception... "Not enough arguments [nsIStreamListener.onDataAvailable]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: file:///builds/worker/workspace/build/tests/mochitest/tests/dom/base/test/send_gzip_content.sjs :: gzipCompressString :: line 14" data: no] on line -2516

However if it's not landed at the same time I doubt tests will be happy with the update also. Any ideas on how to land these together?

Flags: needinfo?(jkt) → needinfo?(aerickson)

I'm not familiar with any way of ensuring they all land together other than making all of the changes in the same commit (but I'm pretty new).

I'll share a diff with the new hostutils manifests when I'm done so you can add them to your diff.

Flags: needinfo?(aerickson)

If you add them to your bug and get them reviewed I will try and find someone to land them at the same time tomorrow. Thanks!

Flags: needinfo?(aerickson)

:gbrown is there an ordering of the landing of these patches we need to take care of? Should I disable the failing test in this bug so the host utils can be updated and the test re-enabled again?

Flags: needinfo?(gbrown)

I think the landing is working out okay. Andrew will update the Linux host-utils, then you can land this bug, then Andrew will update the osx host-utils. The osx host-utils don't affect continuous integration, so the only consequence of the time lag will be that anyone running Android tests on their osx laptop would see the affected tests fail (probably no one will notice).

Flags: needinfo?(gbrown)
Flags: needinfo?(aerickson)

jkt -- Sorry, I misunderstood the scope of Andrew's try push, so thought everything was working out okay, but it didn't. As you expected, your changes in this bug can't land before the hostutils update and the hostutils update can't land before your changes. If you can arrange to land all changes together, that should be fine. Or you could disable the failing test, land, then re-enable (as you suggested earlier!).

Flags: needinfo?(jkt)

Queued the landing with the test disabled, if it sticks then we can renable after hostutils has been updated. I appreciate you both looking into this!

Thanks!

Flags: needinfo?(jkt)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5f7ff4ca9b5
Removing context from OnStartRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/c48f47f793c0
Removing context from OnStopRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/2f665c88b0c8
Changing js to remove context from onStartRequest and onStopRequest r=valentin
https://hg.mozilla.org/integration/autoland/rev/20fcf9d9494a
Removing context from OnDataAvailable r=valentin
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.