Update sandbox tests to reflect expected plugin behavior

RESOLVED FIXED in mozilla17

Status

()

Core
Identity
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: johns, Assigned: johns)

Tracking

Trunk
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

5 years ago
Currently in toolkit/identity/tests/chrome/test_sandbox.xul we test that objects are suppressed by checking for channel opens.

However, in bug 745030, we become slightly smarter about opening channels for <object>/<embed> tags - we will open the channel if the server might return a unblocked MIME type (e.g. if image, subdocument, OR plugin is allowable), and then abort if it returns a blocked type. This means that checking for a channel open isn't sufficient for testing whether the object finished loading.
(Assignee)

Comment 1

5 years ago
Created attachment 644117 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior
(Assignee)

Comment 2

5 years ago
Comment on attachment 644117 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior

This changes the channel checking slightly to be okay with plugin types, but then adds a new block to specifically check that no plugin tags loaded any content (via nsIObjectLoadingContent.displayedType == TYPE_NULL)
Attachment #644117 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 644117 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior

Review of attachment 644117 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this. r=me with fixes.

::: toolkit/identity/tests/chrome/test_sandbox.xul
@@ +128,5 @@
>  
>      if (aNothingShouldLoad) {
>        is(xhr.responseText, "NOTHING", "Check that nothing was loaded on the server");
>      } else if (!OSX_10_5) {
> +    } else {

I'm guessing this didn't get rebased properly. Remove the |} else {| line so that these checks stay disabled on 10.5 due to intermittent failures (bug 770063).

@@ +135,4 @@
>        let loadedTypes = xhr.responseText == "NOTHING" ? [] : xhr.responseText.split(",");
>  
> +      for (let ind in loadedTypes) {
> +        isnot(allowedTypes.indexOf(loadedTypes[ind]), -1, "Check that " + loadedTypes[ind] + " was expected to load");

You can use for…of here since you are only using |ind| to get loadedTypes[ind]:

|for (let loadedType of loadedTypes) {|

https://developer.mozilla.org/en/JavaScript/Reference/Statements/for...of

@@ +154,5 @@
> +      let nullType = Components.interfaces.nsIObjectLoadingContent.TYPE_NULL;
> +      let pluginTags = [ "embed", "object", "applet" ];
> +      for (let ind = 0; ind < pluginTags.length; ind++) {
> +        let tags = doc.getElementsByTagName(pluginTags[ind]);
> +        for (let tagInd = 0; tagInd < tags.length; tagInd++) {

I think this would be clearer with one loop using querySelectorAll and for…of:

for (let tag of doc.querySelectorAll("embed, object, applet")) {
  tag instanceof Components.interfaces.nsIObjectLoadingContent;
  is(tag.displayedType, nullType, "Check that plugin did not load content");
}
Attachment #644117 - Flags: review?(mnoorenberghe+bmo) → review+
(Assignee)

Comment 4

5 years ago
Created attachment 645574 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior, r=mattn
(Assignee)

Updated

5 years ago
Attachment #644117 - Attachment is obsolete: true
(Assignee)

Comment 5

5 years ago
for..of? querySelectorAll? That will NEVER work in IE7!

Oh. Right.

Updated with notes, I'll land this tomorrow with bug 745030 if nothing else burns down
(Assignee)

Comment 6

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c697eeb050dd
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d5231ba7849
https://hg.mozilla.org/integration/mozilla-inbound/rev/aef63a0ce60c
(Assignee)

Comment 8

5 years ago
This was backed out along with bug 745030 for random oranges, though 745030 is likely to blame:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13845528&tree=Mozilla-Inbound&full=1
(Assignee)

Comment 9

5 years ago
Created attachment 648507 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior. r=mattn

Minor tweak, carrying forward r+
(Assignee)

Comment 10

5 years ago
Created attachment 648513 [details] [diff] [review]
Update sandbox test to reflect expected plugin behavior. r=mattn

Minor tweak v2!
Attachment #645574 - Attachment is obsolete: true
Attachment #648507 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f47de086284
(Assignee)

Comment 12

5 years ago
Annnd backed out again for aggravating Bug 778179 go nuts...

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf46b1ea1ccc
(Assignee)

Comment 13

5 years ago
(In reply to John Schoenick [:johns] from comment #12)
> Annnd backed out again for aggravating Bug 778179 go nuts...

Err, making* bug 778179 go nuts
I'll look at that tomorrow.  If you can catch the crash in a debugger, that would help.
(Assignee)

Comment 15

5 years ago
Third time's the charm, now that we think 778179 has been resolved

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bfbd9a8ed90
https://hg.mozilla.org/mozilla-central/rev/3bfbd9a8ed90
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.