install.rdf parsing incorrectly assumes that tags always have children

RESOLVED FIXED

Status

Testing
Talos
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Wladimir Palant, Assigned: alice)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mozbase])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
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)
(Reporter)

Updated

7 years ago
Blocks: 599169, 648784
(Assignee)

Updated

7 years ago
Assignee: nobody → anodelman
(Assignee)

Comment 1

7 years ago
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?
(Assignee)

Comment 2

7 years ago
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?
(Assignee)

Comment 3

7 years ago
Created attachment 550812 [details] [diff] [review]
handle elements with no children more gracefully

Even if no addons use that format, talos should still handle the edge case more gracefully.
Attachment #550812 - Flags: review?(jmaher)
(Reporter)

Comment 4

7 years ago
(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 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.
Attachment #550812 - Flags: review?(jmaher) → review+
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
Mossop - I was told on irc that you may know the answer to comment#6.
(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.
(Assignee)

Comment 9

7 years ago
Okay, I've changed the default to 'false' in the case of no true/false indication when the unpack tag is present.
(Assignee)

Comment 10

7 years ago
Created attachment 554131 [details] [diff] [review]
[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.
Attachment #550812 - Attachment is obsolete: true
(Assignee)

Comment 11

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

changeset:   246:f1e081446e15
Attachment #554131 - Attachment description: handle elements with no children more gracefully (take 2) → [checked in]handle elements with no children more gracefully (take 2)
(Assignee)

Updated

7 years ago
Depends on: 680153
(Assignee)

Updated

7 years ago
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 12

7 years ago
this could affect MozBase going forward once we start to unify our python harnesses, so whiteboarding for triage
Whiteboard: [mozbase]
(Assignee)

Comment 13

7 years ago
Created attachment 554164 [details] [diff] [review]
[checked in]bustage fix - need to look at childNodes not list of tags
Attachment #554164 - Flags: review?(jmaher)
(Assignee)

Comment 14

7 years ago
Bustage found during additional testing - was searching for text in lists of tags instead of list of child nodes.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Attachment #554164 - Flags: review?(jmaher) → review+
(Assignee)

Comment 16

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

changeset:   247:d101b206d149
Attachment #554164 - Attachment description: bustage fix - need to look at childNodes not list of tags → [checked in]bustage fix - need to look at childNodes not list of tags
(Assignee)

Comment 17

7 years ago
Bustage fix has been checked in and deployed.

All done here.
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.