Closed Bug 792269 Opened 12 years ago Closed 12 years ago

error: api-utils: TEST FAILED: test-frame-utils.test frame creation (exception)

Categories

(Add-on SDK Graveyard :: General, defect)

Product:
Add-on SDK Graveyard
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: KWierso, Unassigned)

References

Details

Attachments

(1 file)

Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/583
355 bytes, text/html
mossop
: review+
Details 
info: api-utils: executing 'test-frame-utils.test frame creation'
error: api-utils: TEST FAILED: test-frame-utils.test frame creation (exception)
error: api-utils: An exception occurred.
Traceback (most recent call last):
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/timer.js", line 28, in notify
    callback.apply(null, args);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/unit-test.js", line 263, in done/<
    timer.setTimeout(function() { onDone(self); }, 0);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/unit-test.js", line 415, in runNextTest
    self.start({test: test, onDone: runNextTest});
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/unit-test.js", line 438, in start
    this.test.testFunction(this);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/unit-test-finder.js", line 25, in runTest
    test(runner);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/test.js", line 68, in 
    test(assert);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/tests/test-frame-utils.js", line 12, in exports["test frame creation"]
    let frame = create(window.document);
  File "resource://3c21b69a-b8a1-4e1b-acd2-41c4b3540d27-at-jetpack/api-utils/lib/frame/utils.js", line 52, in create
    docShell.allowAuth = options.allowAuth || false;
TypeError: docShell is undefined


This showed up after bug 789773 landed, and it landed to inbound (and soon central), aurora and beta.

Note, it's harder to see on inbound/central because I haven't pushed the change over to pick up the fix to the previous bustage, but it's there, too.

https://tbpl.mozilla.org/php/getParsedLog.php?id=15319519&tree=Mozilla-Inbound&full=1

https://tbpl.mozilla.org/php/getParsedLog.php?id=15318499&tree=Mozilla-Aurora&full=1
OS: Windows 8 → All
Hardware: x86_64 → All
Ochameau: could you please give us statistics on how many add-ons depend on this module to estimate an impact ?

Thanks
It's unclear to me from bug 789773 comment 91 whether this is an issue in the jetpack tests or an actual issue in the SDK that's going to break addons. Can you clarify?

If the impact is large, we need to think about what to do here, given that this is now on beta which is shipping Real Soon Now. Poking around at the relevant platform code seems to always break something new, so doing so isn't ideal. But given the way addon packing works, we don't have a great shot at fixing this in real addons before release either, do we?
None of SDK addons are using this frame/utils module.
None of SDK modules are using it either.

I'd suggest to drop this module until we start using content/remote iframes.
There is already better code than our in m-c:
  http://mxr.mozilla.org/mozilla-central/source/toolkit/identity/Sandbox.jsm#83

But I was more concerned about the general issue it can cause to docShell being missing on <iframe>. We are accessing docShell attribute in other API, like here:
  https://github.com/mozilla/addon-sdk/blob/master/packages/addon-kit/lib/panel.js#L334
And here:
  https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/content/symbiont.js#L108

But, the following testcase highlights that the docShell is missing on these problematic builds only when we create an iframe on a still-loading toplevel chrome window. The good thing is that, in these two modules (symbiont and panel), we are creating an iframe either on the hidden window or a browser window. And I'm expecting that both are already loaded before creation the iframe.


  let Cc = Components.classes;
  let Ci = Components.interfaces;

  let XUL = 'http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul';

  let windowWatcher = Cc['@mozilla.org/embedcomp/window-watcher;1']
                        .getService(Ci.nsIWindowWatcher);                    
  let win = windowWatcher.
            openWindow(null, 'data:text/html;charset=utf-8,foo', null, '', null);

  //win.addEventListener("load", function () {

    let frame = win.document.createElementNS(XUL, 'browser');
    frame.setAttribute('type', 'content');
    frame.setAttribute('src', 'about:blank');
    
    win.document.documentElement.appendChild(frame);
    
    Components.utils.reportError(frame.docShell);

  //}, true);

This test case works as-is in release build but fails on inbound build.
It works fine on inbound if you uncomment `load` event listening.


Conclusion: no particular issue for jetpack from what I can see. But doesn't it highlight some issue on this about:blank document? Why does it work fine without this patch? Isn't there something missing in this new default about:blank document compared to the old double messy one?
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> None of SDK addons* are using this frame/utils module.

* hosted on AMO!
(In reply to Alexandre Poirot (:ochameau) from comment #3)
> Conclusion: no particular issue for jetpack from what I can see. But doesn't
> it highlight some issue on this about:blank document? Why does it work fine
> without this patch? Isn't there something missing in this new default
> about:blank document compared to the old double messy one?

Good question. I'm going to take a quick look at this.
(In reply to Bobby Holley (:bholley) from comment #5)
> Good question. I'm going to take a quick look at this.

A quick look took too long and I'm getting pulled away to other things. It looks like browser.xul isn't properly setting up the docshell when the <constructor> runs, but I don't have time to debug it at the moment unless we believe it's a serious issue.
(In reply to Bobby Holley (:bholley) from comment #2)
> It's unclear to me from bug 789773 comment 91 whether this is an issue in
> the jetpack tests or an actual issue in the SDK that's going to break
> addons. Can you clarify?
> 
> If the impact is large, we need to think about what to do here, given that
> this is now on beta which is shipping Real Soon Now. Poking around at the
> relevant platform code seems to always break something new, so doing so
> isn't ideal. But given the way addon packing works, we don't have a great
> shot at fixing this in real addons before release either, do we?

It looks like we won't have any impact on AMO hosted add-on's so we'll just update implementation details to fix our tests and make sure we don't create elements in the intermediate documents.
Comment on attachment 662721 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/583

><!DOCTYPE html><meta charset="utf-8"><meta http-equiv="refresh" content="5;https://github.com/mozilla/addon-sdk/pull/583"><title>Bugzilla Code Review</title><p>You can review this patch at <a href="https://github.com/mozilla/addon-sdk/pull/583">https://github.com/mozilla/addon-sdk/pull/583</a>, or wait 5 seconds to be redirected there automatically.</p>
Attachment #662721 - Flags: review?(dtownsend+bugmail)
Comment on attachment 662721 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/583

Looks ok and can land to clean up the test failure. Please also add a guard in frame/utils.create to check the document is loaded and throw a sensible error if not.
Attachment #662721 - Flags: review?(dtownsend+bugmail) → review+
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a06fed4a80671ad78e9d8404625157e0cd7102d5
Merge pull request #583 from Gozala/bug/frame-utils-test@792269

Bug 792269 - Update test to make use of frame/utils on loaded document only. r=@Mossop
https://hg.mozilla.org/mozilla-central/rev/447705d39456
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4b7942c1ad5cfbd3b6223dc3439987367056570e
Merge pull request #583 from Gozala/bug/frame-utils-test@792269

Bug 792269 - Update test to make use of frame/utils on loaded document only. r=@Mossop(cherry picked from commit a06fed4a80671ad78e9d8404625157e0cd7102d5)
Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/4b7942c1ad5cfbd3b6223dc3439987367056570e
Merge pull request #583 from Gozala/bug/frame-utils-test@792269
Commit pushed to stabilization at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a06fed4a80671ad78e9d8404625157e0cd7102d5
Merge pull request #583 from Gozala/bug/frame-utils-test@792269
Commit pushed to release at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/a06fed4a80671ad78e9d8404625157e0cd7102d5
Merge pull request #583 from Gozala/bug/frame-utils-test@792269
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: