1073259 - Make SettingsManager calls with invalid windows throw better errors

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

As of bug 900551 landing, if mozSettings.createLock() is called after inner-window-destroyed, an exception is thrown due to sendAsyncMessage (called from the SettingsLock constructor) needing a window. The prior version of createLock was always guaranteed to succeed (and many calls expect it to, since they're made in a chained way, i.e. mozSettings.createLock().get(whatever), so the current one should too.

This should be fixable by moving the createLock message to be sent the first time SettingsLock.sendMessage is called. Then we can always pass back a lock, and fail all requests after if it has no window to send to.

Comment 1

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Actually, there's a far more simple fix: If we don't have a window, just return. We already do this in _closeHelper, but it should happen in sendMessage. This will make chained calls to get/set fail catastrophically when something tries to create a DOMRequest, but that should possibly be handled by DOMRequestHelper.

Comment 2

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 1 (v1) - Print more descriptive errors when DOMRequestHelper tries to make request with invalid window

Comment 3

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Attachment: Patch 2 (v1) - Print more descriptive errors when SettingsManager tries to make request with invalid window

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

These patches should make errors more obvious, but do not fix the fact that Settings queries can fail after inner-window-destroyed. Best we can do there is a message to gaia.

Comment 5

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8495625 [details] [diff] [review]
Patch 2 (v1) - Print more descriptive errors when SettingsManager tries to make request with invalid window

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

::: dom/settings/SettingsManager.js
@@ +111,5 @@
> +    // before sending, otherwise just ignore.
> +    if (!this._settingsManager._window) {
> +      Cu.reportError("SettingsManager window died, cannot run settings transaction.");
> +      Cu.reportError("SettingsMessage: " + aMessageName);
> +      Cu.reportError("SettingsData: " + JSON.stringify(aData));

This should probably be just one long string rather than 3 callse to reportError().

Comment 6

[:gerard-majax]

Yay ... attachment 8495625 [details] [diff] [review] may help in bug 1074379

Comment 7

[:gerard-majax]

Attachment: 0001-Bug-1073529-Print-more-descriptive-errors-when-Setti.patch

Carrying r+ from :bent

Can we land this ?

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Alexandre LISSY :gerard-majax from comment #7)
> Created attachment 8504146 [details] [diff] [review]
> 0001-Bug-1073529-Print-more-descriptive-errors-when-Setti.patch
> 
> Carrying r+ from :bent
> 
> Can we land this ?

Sure, go ahead!

Comment 9

[Johan Lorenzo [:jlorenzo]]

(In reply to Alexandre LISSY :gerard-majax from comment #6)
The issue from bug 1074379 is still present with that patch :S

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there a Try run for this?

Comment 11

[Ryan VanderMeulen [:RyanVM]]

And do both patches need to land?

Comment 12

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> Is there a Try run for this?

https://tbpl.mozilla.org/?tree=Try&rev=f98b982e8afb
https://tbpl.mozilla.org/?tree=Try&rev=513ebd39b774

Comment 13

[:gerard-majax]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> And do both patches need to land?

I think so.

Comment 14

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #12)
> (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #10)
> > Is there a Try run for this?
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f98b982e8afb
> https://tbpl.mozilla.org/?tree=Try&rev=513ebd39b774

Ryan, those are finished, it's mostly green but you may want to have a look to make sure :)

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks good to me. Please fix the bug number in the commit messages and request checkin.

Comment 16

[:gerard-majax]

Oh, I missed this, thanks for catching.

Comment 17

[:gerard-majax]

Attachment: Print more descriptive errors when SettingsManager tried to make request with invalid window r=bent

Comment 18

[:gerard-majax]

Attachment: Print more descriptive errors when DOMRequestHelper tries to create request with invalid window r=fabrice

Comment 19

[:gerard-majax]

Comment on attachment 8506872 [details] [diff] [review]
Print more descriptive errors when SettingsManager tried to make request with invalid window r=bent

Carrying r+ from :bent, fixing bug number in commit message.

Comment 20

[:gerard-majax]

Comment on attachment 8506873 [details] [diff] [review]
Print more descriptive errors when DOMRequestHelper tries to create request with invalid window r=fabrice

Carrying r+ from :fabrice, fixing bug number in commit message.

Comment 21

[:gerard-majax]

Everything should be good now, fixed the commit messages, thanks for catching this!

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/b73bb9d2c9e1
https://hg.mozilla.org/integration/b2g-inbound/rev/3bd9a78ed444

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b73bb9d2c9e1
https://hg.mozilla.org/mozilla-central/rev/3bd9a78ed444