1232631 - Write tests for network part of bug 1137681

Status: NEW

(Core :: Networking: HTTP, defect, P5)

Description

[Tim Nguyen :ntim]

The docshell part of the patch already has a test, but not the network part.

Comment 1

[Parker]

How would I go about testing this?

Comment 2

[Tim Nguyen :ntim]

(In reply to Parker from comment #1)
> How would I go about testing this?

Thanks for your interest!
What the test needs to test:
There's a flag on the docShell (window wrapper) that allows you to set the user agent globally for a tab. That flag is customUserAgent. You should test whether that flag applies to the network requests sent inside that tab.

To write the actual test:
- in netwerk/test/mochitests, create a new file called test_user_agent_docshell_override.html
- Paste in the contents of netwerk/test/mochitests/test_user_agent_updates_reset.html inside the file you created
- Add the test to netwerk/test/mochitests/mochitest.ini
- Then you should be able to modify the test to check if the docshell override works, for that task, you can use the following:

... to set the docShell override:
function setOverride(override)
  var Ci = Components.interfaces;
  var docShell = SpecialPowers.wrap(window).QueryInterface(Ci.nsIInterfaceRequestor)
                                 .getInterface(Ci.nsIWebNavigation)
                                 .QueryInterface(Ci.nsIDocShell);
  docShell.customUserAgent = override;
}

... to test if the override works:
isnot(getUA(location.origin), UA_OVERRIDE,
      "No override is set yet");

setOverride(UA_OVERRIDE);

is(getUA(location.origin), UA_OVERRIDE,
  "User-Agent should be");

You'll also need to run the test to see if it works, for that, do:
./mach mochitest netwerk/test/mochitests/test_user_agent_updates_reset.html
Make sure all the tests pass, if not, feel free to needinfo me.

Comment 3

[Steve Jarvis [:sajarvis]]

Hey, the netwerk/test/mochitests/user_agent_update.sjs changed in February to 'postMessage' to parent page instead of writing only the UA in the response (rev https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1). So, test_user_agent_updates_reset.html gets the whole page as the "UA" (script tags and all) and fails the test.

There seem like two possible best fixes:

1- Simpler option, add another .sjs file that writes only the UA in the response and make the request to that. Just as user_agent_update.sjs did before rev 11a7fbfbe2d1.
2- Change test_user_agent_updates_reset.html (and the new docshell test) to use an embedded iFrame and handle the postMessage event.

I'm guessing #2, since the other tests moved that way for a reason, but what's the right path forward?

Comment 4

[Tim Nguyen :ntim]

(In reply to Steve Jarvis [:sajarvis] from comment #3)
> Hey, the netwerk/test/mochitests/user_agent_update.sjs changed in February
> to 'postMessage' to parent page instead of writing only the UA in the
> response (rev https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1). So,
> test_user_agent_updates_reset.html gets the whole page as the "UA" (script
> tags and all) and fails the test.
Thanks for looking into this bug.

I'm surprised test_user_agent_updates_reset.html fails, since it had never been disabled (which means any failure should have been caught by CI). It probably fails silently now.
Anyway, that test should have been updated as per bug 1148544 as well, because right now it tests nothing. 

> There seem like two possible best fixes:
> 
> 1- Simpler option, add another .sjs file that writes only the UA in the
> response and make the request to that. Just as user_agent_update.sjs did
> before rev 11a7fbfbe2d1.
> 2- Change test_user_agent_updates_reset.html (and the new docshell test) to
> use an embedded iFrame and handle the postMessage event.
> 
> I'm guessing #2, since the other tests moved that way for a reason, but
> what's the right path forward?
#2 is the way to go, but let's check with Dylan who has more background than I do on this change.

Comment 5

[Steve Jarvis [:sajarvis]]

(In reply to Tim Nguyen :ntim (use needinfo?) from comment #4) 
> I'm surprised test_user_agent_updates_reset.html fails, since it had never
> been disabled (which means any failure should have been caught by CI). It
> probably fails silently now.
> Anyway, that test should have been updated as per bug 1148544 as well,
> because right now it tests nothing. 

Yeah I worded that poorly, test_user_agent_updates_reset.html only checks that the user agent set is *not* the override, and "<html><body><script..." is successfully != "DummyUserAgent". So the test doesn't technically fail, but it doesn't work as intended, and using it as a framework in the docshell test to measure equality does fail.

> #2 is the way to go, but let's check with Dylan who has more background than I do on this change.

Great, thanks!

Comment 6

[Dylan Roeh (:droeh) (he/him)]

Yes, I think #2 is the way to go. If I'm recalling correctly, the reason I had to make those changes to the test cases was that an XHR is a sub-resource request, and will inherit the cached UA override (unless a UA is explicity provided in the XHR) rather than getting a new one.

Comment 7

[pkssg2006]

is this still available for anyone to take ?

Comment 8

[Steve Jarvis [:sajarvis]]

(In reply to pkssg2006 from comment #7)
> is this still available for anyone to take ?

Hey pkssg2006, yeah, I didn't do any further work on it, feel free to take over.

Comment 9

[Nick Fox [:njfox]]

pkssg2006 have you made any progress on this one? I was considering taking a stab at it too.

Comment 10

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Sample test_user_agent_updates_reset.html file

Going by Steve's comments, I tried to come up with a revised test_user_agent_updates_reset file.I'm not sure of how to change the code below to suit the new function added:

isnot(getUA(location.origin), UA_OVERRIDE,
  "User-Agent is not reverted");

If this looks okay or after recommendations from you guys, I'll then modify the test_user_agent_docshell_override.html file to reflect what's in test_user_agent_updates_reset.

Comment 11

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8856100 [details]
Sample test_user_agent_updates_reset.html file

I'm not sure I'm really the best person to review this. I made some small changes to the UA override tests to let some Android-specific changes pass, but I'm not particularly comfortable/familiar with the code.

Off the top of my head, smaug might be a better choice to review this.

Comment 12

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Leni Kadali from comment #10)
> Created attachment 8856100 [details]
> Sample test_user_agent_updates_reset.html file
> 
> Going by Steve's comments, I tried to come up with a revised
> test_user_agent_updates_reset file.I'm not sure of how to change the code
> below to suit the new function added:
> 
> isnot(getUA(location.origin), UA_OVERRIDE,
>   "User-Agent is not reverted");
> 
> If this looks okay or after recommendations from you guys, I'll then modify
> the test_user_agent_docshell_override.html file to reflect what's in
> test_user_agent_updates_reset.

Smaug, could you kindly take a look at this?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Not sure what I'm supposed to look at. I'd expect some mochitest setting UA string to various values and testing it works when doing requests.

Comment 14

[Leni Kadali [:leni1] [GMT+3]]

(In reply to Leni Kadali from comment #10)
> Created attachment 8856100 [details]
> Sample test_user_agent_updates_reset.html file
> 
> Going by Steve's comments, I tried to come up with a revised
> test_user_agent_updates_reset file.I'm not sure of how to change the code
> below to suit the new function added:
> 
> isnot(getUA(location.origin), UA_OVERRIDE,
>   "User-Agent is not reverted");
> 
> If this looks okay or after recommendations from you guys, I'll then modify
> the test_user_agent_docshell_override.html file to reflect what's in
> test_user_agent_updates_reset.

Apologies, I should have quoted the right message. As per the attachment, I had created a sample test_user_agent_updates_reset.html file. However I was stuck at how to rewrite the function mentioned above since Dylan had made this change: https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1. So when I first made my needinfo request, I addressed it to Tim who is the mentor of the bug, who then passed it on to Dylan since he knew more about the change, who then suggested smaug as a better person to ask. As mentioned in IRC, he forgot to pass on the needinfo request as part of his comment. So I 'manually' passed it on but without the requisite detail. Hope it explains what you're to look at :)

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Better to ask Tim then.  I'm not familiar with bug 1137681 and I haven't seen any patch here to review, nor really a question to answer.
Sorry that I can't be too useful here.

Comment 16

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Write tests for network part of bug 1137681

Removed the DOCTYPE declaration
Sample test_user_agent_updates_reset.html file
Going by Steve's comments, I tried to come up with a revised test_user_agent_updates_reset.html file.
Not sure of how to change the code below to suit the new function added:

isnot(getUA(location.origin), UA_OVERRIDE,
  "User-Agent is not reverted");

If this is okay, I'll then modify the test_user_agent_docshell_override.html file to reflect what's in test_user_agent_updates_reset.html

Comment 17

[Tim Nguyen :ntim]

The patch looks empty to me. Are you sure you've generated it properly ?

Comment 18

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Bug 1232631 - Write tests for network part of bug 1137681

Edited some formatting. Otherwise the file remains unchanged.
I wanted help modifying the function
isnot(getUA(location.origin), UA_OVERRIDE,
   "User-Agent is not reverted");
since both Dylan and Olli say they are not familiar enough with the code
to propose how the function should be modified to suit the comments made
starting here: https://bugzilla.mozilla.org/show_bug.cgi?id=1232631#c3

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

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8863118 [details]
Bug 1232631 - Write tests for network part of bug 1137681

https://reviewboard.mozilla.org/r/134946/#review137894

The diff doesn't show much for me, only a blank line removal.

Comment 20

[Leni Kadali [:leni1] [GMT+3]]

Comment on attachment 8863118 [details]
Bug 1232631 - Write tests for network part of bug 1137681

https://reviewboard.mozilla.org/r/134946/#review137894

Yes. I know. I need you to review the entire file. See if it meets the requirements of the bug. I haven't managed to get help regarding the modification of the function

isnot(getUA(location.origin), UA_OVERRIDE,
   "User-Agent is not reverted");
   
 If there's anything else I need to add or do, let me know.

Comment 21

[Tim Nguyen :ntim]

(In reply to Leni Kadali from comment #20)
> Comment on attachment 8863118 [details]
> Bug 1232631 - Write tests for network part of bug 1137681
> 
> https://reviewboard.mozilla.org/r/134946/#review137894
> 
> Yes. I know. I need you to review the entire file. See if it meets the
> requirements of the bug. I haven't managed to get help regarding the
> modification of the function
It's quite hard to review a modification without having a diff of the changes.
You can read the docs here: https://mozilla-version-control-tools.readthedocs.io/en/latest/hgmozilla/index.html
or request help from IRC: https://wiki.mozilla.org/IRC

Comment 22

[Leni Kadali [:leni1] [GMT+3]]

Attachment: Write tests for network part of bug 1137681

Modified netwerk/test/mochitests/test_user_agent_updates_reset.html
in line with changes made here: https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1
Discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1232631#c3

I would like assistance on how to modify the code below to reflect the changed function:

const UA_OVERRIDE = "DummyUserAgent";

info("User agent is " + navigator.userAgent);
isnot(navigator.userAgent, UA_OVERRIDE,
  "navigator.userAgent is not reverted");
isnot(getUA(location.origin), UA_OVERRIDE,
  "User-Agent is not reverted");

This is preliminary work to ensure that there's a good base for the docshell test.

MozReview-Commit-ID: EAFIcRcdZme

Comment 23

[Leni Kadali [:leni1] [GMT+3]]

Attachment: sample test_user_agent_updates_reset.html file

Write tests for network part of bug 1137681

Modified netwerk/test/mochitests/test_user_agent_updates_reset.html
in line with changes made here: https://hg.mozilla.org/mozilla-central/rev/11a7fbfbe2d1
Discussion here: https://bugzilla.mozilla.org/show_bug.cgi?id=1232631#c3

I would like assistance on how to modify the code below to reflect the changed function:

const UA_OVERRIDE = "DummyUserAgent";

info("User agent is " + navigator.userAgent);
isnot(navigator.userAgent, UA_OVERRIDE,
  "navigator.userAgent is not reverted");
isnot(getUA(location.origin), UA_OVERRIDE,
  "User-Agent is not reverted");

This is preliminary work to ensure that there's a good base for the docshell test.

For some reason, I was unable to properly export this to Bugzilla or submit it to MozReview. I didn't realize that the file I had submitted to MozReview was the older version of the file, so my apologies Tim :). I'm having growing pains as regards my abilities to successfully diff and submit patches. 

I'm needinfo-ing smaug so that he can review this, since the last time I submitted a file for review it was the wrong file. :P

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Tim reviewing the test should be fine, but that patch still isn't a patch. It is just a file, without changes to relevant moz.build or so.
If MozReview doesn't work, one can always upload patches to bugzilla as attachments and ask reviews.

The test doesn't wait for the message event to arrive, so depending on timing it may fail.
You want some SimpleTest.waitForExplicitFinish(); and SimpleTest.finish() use.
And nothing calls testUAIFrame, so the test doesn't do anything currently.

Comment 25

[Tim Nguyen :ntim]

Comment on attachment 8863756 [details] [diff] [review]
sample test_user_agent_updates_reset.html file

> I'm having growing pains as regards my abilities to successfully diff and submit patches. 

You can request help from the #developers channel on irc.mozilla.org if needed.

It's a bit hard to review a file by itself, without seeing the changes that were made.

Comment 26

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 27

[Michael Bailey]

Attachment: same problem

Thanks from cambsorino1975.