658187 - install.rdf parsing incorrectly assumes that tags always have children

Status: RESOLVED FIXED

(Testing :: Talos, defect)

Description

[Wladimir Palant]

I noticed that the tests for ReminderFox fail, see for example http://tinderbox.mozilla.org/showlog.cgi?log=AddonTester/1305382569.1305383175.15192.gz:

Traceback (most recent call last):
  File "run_tests.py", line 569, in ?
    test_file(arg, screen, amo)
  File "run_tests.py", line 514, in test_file
    browser_dump, counter_dump, print_format = mytest.runTest(browser_config, test)
  File "c:\talos-slave\talos-data\talos\ttest.py", line 235, in runTest
    profile_dir, temp_dir = self.createProfile(test_config['profile_path'], browser_config)
  File "c:\talos-slave\talos-data\talos\ttest.py", line 134, in createProfile
    browser_config['extensions'])
  File "c:\talos-slave\talos-data\talos\ffsetup.py", line 239, in CreateTempProfileDir
    self.install_addon(profile_dir, addon)
  File "c:\talos-slave\talos-data\talos\ffsetup.py", line 183, in install_addon
    unpack = find_unpack(desc)
  File "c:\talos-slave\talos-data\talos\ffsetup.py", line 158, in find_unpack
    unpack = str(elem.getElementsByTagName('em:unpack')[0].firstChild.data)
AttributeError: 'NoneType' object has no attribute 'data'

install.rdf of this extension starts like this:

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:RDF="http://www.w3.org/1999/02/22-rdf-syntax-ns#" xmlns:em="http://www.mozilla.org/2004/em-rdf#">
    <RDF:Description about="urn:mozilla:install-manifest">
		<em:id>{ada4b710-8346-4b82-8199-5de2b400a6ae}</em:id>
		<em:name>ReminderFox</em:name>
		<em:version>1.9.9.3.1</em:version>
		<em:description>Displays and manages reminders and ToDo's</em:description>
		<em:creator>Tom Mutdosch and Daniel Lee</em:creator>
		<em:contributor>Guenter Wahl</em:contributor>
		<em:iconURL>chrome://reminderfox/skin/images/foxy-head.png</em:iconURL>
		<em:aboutURL>chrome://reminderfox/content/about.xul</em:aboutURL>
		<em:unpack/>
		<em:type>2</em:type>

As you can see, the unpack tag has no children so getting firstChild.data throws an exception. Which is why I use the following function to get node text:

  def getText(node):
    result = []
    for child in node.childNodes:
        if child.nodeType == child.TEXT_NODE:
            result.append(child.data)
    return ''.join(result)

Comment 1

[alice nodelman [:alice] [:anode]]

In the example that you provided does that indicate that an unpack should or should not happen?  Does the mere presence of the element 'unpack/' mean that it should be installed unpacked?  Or does having neither a true/false value mean that it defaults to false?

Comment 2

[alice nodelman [:alice] [:anode]]

I also pulled the current version of ReminderFox and it now uses:

    <RDF:Description about="urn:mozilla:install-manifest">
                <em:id>{ada4b710-8346-4b82-8199-5de2b400a6ae}</em:id>
                <em:name>ReminderFox</em:name>
                <em:version>1.9.9.4</em:version>
                <em:description>Displays and manages reminders and ToDo's</em:description>
                <em:creator>Tom Mutdosch and Daniel Lee</em:creator>
                <em:contributor>Guenter Wahl</em:contributor>
                <em:iconURL>chrome://reminderfox/skin/images/foxy-head.png</em:iconURL>
                <em:aboutURL>chrome://reminderfox/content/about.xul</em:aboutURL>
                <em:unpack>true</em:unpack>
                <em:type>2</em:type>
                <em:file>

Are there any addons that use the <em:unpack/> format?

Comment 3

[alice nodelman [:alice] [:anode]]

Attachment: handle elements with no children more gracefully

Even if no addons use that format, talos should still handle the edge case more gracefully.

Comment 4

[Wladimir Palant]

(In reply to alice nodelman [:alice] [:anode] from comment #1)
> In the example that you provided does that indicate that an unpack should or
> should not happen?  Does the mere presence of the element 'unpack/' mean
> that it should be installed unpacked?  Or does having neither a true/false
> value mean that it defaults to false?

From what I remember, the extension manager wants the unpack value to be "true" - everything else (including an empty value) is interpreted as "false". But I don't have time to verify this in the extension manager code right now.

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 550812 [details] [diff] [review]
handle elements with no children more gracefully

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

I don't know if this change can be pushed up to the mozmill/mozprofile addons stuff, but it would be nice if we could do that as well.

::: ffsetup.py
@@ +169,4 @@
>                  elif elem.hasAttribute('NS1:unpack'):
>                      unpack = str(elem.getAttribute('NS1:unpack'))
> +                if not unpack:  #no value in attribute/elements, defaults to true 
> +                    unpack = 'true'

this is confusing.  We set unpack to false and if there is anything in desc we set it to true or a real value.  Could jut be a lack of understanding of the .rdf files.

Comment 6

[alice nodelman [:alice] [:anode]]

There is still some confusion here as to whether or not just having the unpack tag with no true/false value should result in a default of true or false.  In my patch I assumed it to be true, but it sounds like from comment#4 that it could be false.

This would also clear up the review question in comment#5.

I do not feel comfortable checking this in until we clear up the confusion.

Comment 7

[alice nodelman [:alice] [:anode]]

Mossop - I was told on irc that you may know the answer to comment#6.

Comment 8

[Dave Townsend [:mossop]]

(In reply to alice nodelman [:alice] [:anode] from comment #6)
> There is still some confusion here as to whether or not just having the
> unpack tag with no true/false value should result in a default of true or
> false.  In my patch I assumed it to be true, but it sounds like from
> comment#4 that it could be false.
> 
> This would also clear up the review question in comment#5.
> 
> I do not feel comfortable checking this in until we clear up the confusion.

The tag must contain the text "true" for unpacking to occur.

Of course this is RDF so there are a couple of different ways to specify the same thing but chances are you aren't going to hit any of the others in reality.

Comment 9

[alice nodelman [:alice] [:anode]]

Okay, I've changed the default to 'false' in the case of no true/false indication when the unpack tag is present.

Comment 10

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]handle elements with no children more gracefully (take 2)

Switch default from 'true' to 'false'.

Review carried over from previous patch, based on irc discussion of the change with jmaher.

Comment 11

[alice nodelman [:alice] [:anode]]

Comment on attachment 554131 [details] [diff] [review]
[checked in]handle elements with no children more gracefully (take 2)

changeset:   246:f1e081446e15

Comment 12

[Jeff Hammel]

this could affect MozBase going forward once we start to unify our python harnesses, so whiteboarding for triage

Comment 13

[alice nodelman [:alice] [:anode]]

Attachment: [checked in]bustage fix - need to look at childNodes not list of tags

Comment 14

[alice nodelman [:alice] [:anode]]

Bustage found during additional testing - was searching for text in lists of tags instead of list of child nodes.

Comment 15

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 554164 [details] [diff] [review]
[checked in]bustage fix - need to look at childNodes not list of tags

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

I don't see anything wrong with the code although I am not an expert in rdf files or unpacking addons.

Comment 16

[alice nodelman [:alice] [:anode]]

Comment on attachment 554164 [details] [diff] [review]
[checked in]bustage fix - need to look at childNodes not list of tags

changeset:   247:d101b206d149

Comment 17

[alice nodelman [:alice] [:anode]]

Bustage fix has been checked in and deployed.

All done here.