Widget elements weird content update behavior

RESOLVED FIXED in 1.0

Status

P1
normal
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: peregrino, Assigned: ochameau)

Tracking

unspecified

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Created attachment 495382 [details]
bug testcase

When I try to update a widget's content using html strings like:

my_widget.contet = '<span>test</span>'

the widgets content have different update behavior depending on the html string. 

If the string has an <style> tag, the widget is updated only on uneven times (updates, doesn't update, updates, doesn't update...). Adding weirdness, if you first set the content to a plain text string, and in the next line change it to the desired HTML content, on the uneven times only the plain text is updated, the second set is ignored.

If the string has the styling inline in each element, the content is updated as expected.

To illustrate a little more this bug, I attach a testcase that reproduces the bug.
The steps to use the testcase are:

* run it with the addon-sdk beta 1
* you'll see two buttons, and a third widget with mozilla.org loaded in it
* the buttons are for updating the third widgets content
* when you click on the one that says "style" it loads a file as text from data/ that contains a style tag and feeds that into the third widget
* and the last button does the same, but loading a file with no styles on it
I just tested the case w/ the latest SDK, and the problem still occurs. Just hit the "styles" button repeatedly and you'll see it.
Priority: -- → P1
Target Milestone: --- → 1.0
Dietrich: would you have any time to look into this by any chance?
Well, so far I've gotten down into symbiont and the content is still being passed along properly.
It gets weirder. I think I've narrowed the problem down to the presence of the '#' character in the widget content. Updated testcase coming.
Because the content is set as a data URI, the XUL iframe or browser is not treating the update as a change due to the '#', assuming it's an internal link, i'd wager, and therefore not reloading the content. Or something like that.
More fun facts:

1. For each click of the button, symbiont._initFrame is called *3* times instead of twice. One for the first content change and *twice* for the second content change.

2. At shutdown, symbiont._initFrame is called once for each widget. Which does not immediately appear to make sense.
Aaaaand it's a race. The onReady event handler for the first content change hasn't been received yet, so the second content change short-circuits because the frame location is as yet unchanged.
Awesome. Fixed the short-circuit, so now loading and being notified about both changes. But the bug is still there - definitely related to the #.
We're using encodeURI, so special parts like # weren't getting encoded in the data URI. Switching to encodeURIComponent fixered things right up. Patch up once I get a test written.
Created attachment 530693 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/159

Pointer to Github pull-request
Attachment #530693 - Flags: review?(rFobic)
Assignee: nobody → dietrich
Comment on attachment 530693 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/159

Dietrich, looks good, but I highlighted few nits, can you please fix them before pushing, thanks!
Attachment #530693 - Flags: review?(rFobic) → review+
https://github.com/mozilla/addon-sdk/commit/472797a298ba3b47004ad201acfdd8fd942a098c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
OS: Mac OS X → All
Hardware: x86 → All
Resolution: --- → FIXED
This appears to have broken tests (see Bug 655827 for details), so I backed it out. Be carefuly when re-applying: it'll probably be safest to create a new patch against current master. See the comments in https://github.com/mozilla/addon-sdk/commit/c65f3f7ff9e3abb658b889f4df4b28dcd5fed96c for details.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 15

8 years ago
Created attachment 532730 [details] [diff] [review]
Updated patch.

I fixed the onReady race in bug 652530:
https://github.com/mozilla/addon-sdk/commit/2b6480335bf92419610f452421f511aa41e329d0#packages/api-utils/lib/content/symbiont.js

So the only thing left to do is to use encodeURI instead of encureURIComponent, and add your test.
Attachment #530693 - Attachment is obsolete: true
Attachment #532730 - Flags: review?(rFobic)
Comment on attachment 532730 [details] [diff] [review]
Updated patch.

This change breaks tests (see output below). Also I have a feeling that test is not cleaned up correctly since it seems to break "test-windows.testActiveWindow" and also number of tests without this patch is 1511, with this one it's 1515, so I guess asserts are executed more times than expected. 

.............error: TEST FAILED: test-windows.testActiveWindow (failure)
error: fail: content updated to pound? ("" != "foo#")
info: Traceback (most recent call last):
  File "resource://addon-kit-api-utils-lib/timer.js", line 64, in notifyOnTimeout
    this._callback.apply(null, this._params);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 793, in null
    self._widget._onEvent("message", message);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 424, in WidgetView__onEvent
    this._baseWidget._onEvent(type, eventData);
  File "resource://addon-kit-addon-kit-lib/widget.js", line 300, in _onEvent
    this._emit(type, eventData);
  File "resource://addon-kit-api-utils-lib/events.js", line 147, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://addon-kit-api-utils-lib/events.js", line 177, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://addon-kit-addon-kit-tests/test-widget.js", line 801, in null
    test.assertEqual(getWidgetContent(widget), "foo#", "content updated to pound?")
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 198, in assertEqual
    this.fail(message);
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 116, in fail
    console.trace();

(elapsed time: 50 seconds)
error: fail: Correct number of browser windows (0 != 3)
info: Traceback (most recent call last):
  File "resource://addon-kit-api-utils-lib/tabs/tab.js", line 109, in _onReady
    this._emit(EVENTS.ready.name, this._public);
  File "resource://addon-kit-api-utils-lib/events.js", line 147, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://addon-kit-api-utils-lib/events.js", line 177, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 256, in onReady
    nextStep()
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 265, in nextStep
    testSteps.shift()();
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 202, in null
    test.assertEqual(windows.length, 3, "Correct number of browser windows");
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 198, in assertEqual
    this.fail(message);
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 116, in fail
    console.trace();
error: fail: Correct number of windows returned by iterator (0 != 3)
info: Traceback (most recent call last):
  File "resource://addon-kit-api-utils-lib/tabs/tab.js", line 109, in _onReady
    this._emit(EVENTS.ready.name, this._public);
  File "resource://addon-kit-api-utils-lib/events.js", line 147, in _emit
    return this._emitOnObject.apply(this, args);
  File "resource://addon-kit-api-utils-lib/events.js", line 177, in _emitOnObject
    listener.apply(targetObj, params);
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 256, in onReady
    nextStep()
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 265, in nextStep
    testSteps.shift()();
  File "resource://addon-kit-addon-kit-tests/test-windows.js", line 206, in null
    test.assertEqual(count, 3, "Correct number of windows returned by iterator");
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 198, in assertEqual
    this.fail(message);
  File "resource://addon-kit-api-utils-lib/unit-test.js", line 116, in fail
    console.trace();
..
1514 of 1515 tests passed.
Attachment #532730 - Flags: review?(rFobic) → review-
(Assignee)

Comment 17

8 years ago
Created attachment 534446 [details] [diff] [review]
Unit test fixed.

cfx testall was failing as we didn't destroyed test widget at test end.
Same patch with unit test fixed.
Assignee: dietrich → poirot.alex
Attachment #532730 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attachment #534446 - Flags: review?(rFobic)
Comment on attachment 534446 [details] [diff] [review]
Unit test fixed.

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

Looks good! Alex please remove trailing white-spaces on lines 772 and 778 before landing this.
Attachment #534446 - Flags: review?(rFobic) → review+
(Assignee)

Comment 19

8 years ago
Landed:
https://github.com/mozilla/addon-sdk/commit/718304dd679fe4412f0e26cb8d384917e308ef2a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.